Skip to content

Commit

Permalink
Improve kfctl semantics and ability to rerun app.yaml (kubeflow#4275)
Browse files Browse the repository at this point in the history
* Improve kfctl semantics and ability to rerun app.yaml

* Related to kubeflow/kfctl#49

* Delete NewKfapp its not being called from anywhere.

Refactor NewLoadKfAppFromURI and LoadKfAppCfgFile

* We should only call CreateCfgFile and do the directory check if using a
  remote URI

  * For a local path we already have an appfile so we don't need to recreate
    the CreateCfgFile

  * Calling CreateCfgFile for local paths was a major source of friction
    because it ends up checking if the directory is empty.

    * But when invoking kfctl on a local file we don't need to check if
      the directory is empty because we can assume that's the correct
      ${KFAPP}.

* Once we have a local config file we can treat the two cases the same

* Fix error propogation in apply.go; if BuildKfAppFromURI returns an
error we want to return that error so it ends up being printed out.
  Otherwise we just print out kfApp is nil and swallow the error.

* Improve the error message for project, zone, and config not being set
  by providing the appropriate gcloud command.

* Update kf_is_ready_test.py to try to figure out why things are failing.
  • Loading branch information
jlewi authored and k8s-ci-robot committed Oct 13, 2019
1 parent 9a8a9f7 commit 3b27111
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 131 deletions.
4 changes: 4 additions & 0 deletions bootstrap/cmd/kfctl/cmd/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ var applyCmd = &cobra.Command{

if kind == string(kftypes.KFDEF) {
kfApp, err = coordinator.BuildKfAppFromURI(configFilePath)

if err != nil {
return err
}
} else if kind == string(kftypes.KFUPGRADE) {
kfUpgrade, err := kfupgrade.NewKfUpgrade(configFilePath)
if err != nil {
Expand Down
192 changes: 71 additions & 121 deletions bootstrap/pkg/kfapp/coordinator/coordinator.go
Original file line number Diff line number Diff line change
Expand Up @@ -386,90 +386,15 @@ func isCwdEmpty() string {
// NewLoadKfAppFromURI takes in a config file and constructs the KfApp
// used by the build and apply semantics for kfctl
func NewLoadKfAppFromURI(configFile string) (kftypesv3.KfApp, error) {
url, err := netUrl.ParseRequestURI(configFile)
isRemoteFile := false
cwd := ""
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error parsing config file path: %v", err),
}
} else {
if url.Scheme != "" {
isRemoteFile = true
}
}

// If the config file is downloaded remotely, check to see if the current directory
// is empty because we will be generating the KfApp there.
if isRemoteFile {
cwd = isCwdEmpty()
if cwd == "" {
wd, _ := os.Getwd()
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("current directory %v not empty, please switch directories", wd),
}
}
}

kfDef, err := kfdefsv3.LoadKFDefFromURI(configFile)
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error creating KfApp from config file: %v", err),
}
}

// If the config file is downloaded remotely, use the current working directory to create the KfApp.
// Otherwise use the directory where the config file is stored.
if isRemoteFile {
cwd, err = os.Getwd()
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INTERNAL_ERROR),
Message: fmt.Sprintf("could not get current directory for KfDef %v", err),
}
}
kfDef.Spec.AppDir = cwd
} else {
kfDef.Spec.AppDir = path.Dir(configFile)
}

// basic auth check and warn
useBasicAuth := kfDef.Spec.UseBasicAuth
if useBasicAuth && (os.Getenv(kftypesv3.KUBEFLOW_USERNAME) == "" ||
os.Getenv(kftypesv3.KUBEFLOW_PASSWORD) == "") {
// Printing warning message instead of bailing out as both ENV are used in apply,
// not init.
log.Warnf("you need to set the environment variable %s to the username you "+
"want to use to login and variable %s to the password you want to use.",
kftypesv3.KUBEFLOW_USERNAME, kftypesv3.KUBEFLOW_PASSWORD)
}
// check if zone is set and warn ONLY for GCP
isPlatformGCP := kfDef.Spec.Platform == "gcp"
if isPlatformGCP && os.Getenv("ZONE") == "" {
log.Warn("you need to set the environment variable `ZONE` to the GCP zone you want to use")
}

if kfDef.Spec.PackageManager == "" {
kfDef.Spec.PackageManager = kftypesv3.KUSTOMIZE
}

appFile, err := CreateKfAppCfgFile(kfDef)
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error creating KfApp from config file: %v", err),
}
}
kfApp, err := LoadKfAppCfgFile(appFile)
// TODO(jlewi): Can we merge NewLoadKfAppFromURI and LoadKFAppCfgFile
kfApp, err := LoadKfAppCfgFile(configFile)
if err != nil || kfApp == nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error creating KfApp from config file: %v", err),
}
}

return kfApp, nil
}

Expand Down Expand Up @@ -523,48 +448,6 @@ func CreateKfAppCfgFile(d *kfdefsv3.KfDef) (string, error) {
return cfgFilePath, cfgFilePathErr
}

// NewKfApp is called from the Init subcommand and will create a directory based on
// the path/name argument given to the Init subcommand
func NewKfApp(options map[string]interface{}) (kftypesv3.KfApp, error) {
kfDef, err := CreateKfDefFromOptions(options)

if err != nil {
return nil, err
}

isValid, msg := kfDef.IsValid()

if !isValid {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: msg,
}
}

cfgFilePath, err := CreateKfAppCfgFile(kfDef)

if err != nil {
return nil, err
}

log.Infof("Synchronize cache")

err = kfDef.SyncCache()

if err != nil {
log.Errorf("Failed to synchronize the cache; error: %v", err)
return nil, err
}

// Save app.yaml because we need to preserve information about the cache.
if err := kfDef.WriteToFile(cfgFilePath); err != nil {
log.Errorf("Failed to save KfDef to %v; error %v", cfgFilePath, err)
return nil, err
}

return LoadKfAppCfgFile(cfgFilePath)
}

// backfillKfDefFromInitOptions fills in a KfDef spec based on various command line options.
//
// TODO(jlewi): We should eventually be able to get rid of this function once we remove
Expand Down Expand Up @@ -818,8 +701,52 @@ func GetKfAppFromCfgFile(appFile string, deleteStorage bool) (kftypesv3.KfApp, e

// LoadKfAppCfgFile constructs a KfApp by loading the provided app.yaml file.
func LoadKfAppCfgFile(cfgfile string) (kftypesv3.KfApp, error) {
url, err := netUrl.ParseRequestURI(cfgfile)
isRemoteFile := false
cwd := ""
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error parsing config file path: %v", err),
}
} else {
if url.Scheme != "" {
isRemoteFile = true
}
}

// If the config file is a remote URI, check to see if the current directory
// is empty because we will be generating the KfApp there.
appFile := cfgfile
if isRemoteFile {
cwd = isCwdEmpty()
if cwd == "" {
wd, _ := os.Getwd()
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("current directory %v not empty, please switch directories", wd),
}
}

kfDef, err := kfdefsv3.LoadKFDefFromURI(cfgfile)
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error creating KfApp from config file: %v", err),
}
}

appFile, err = CreateKfAppCfgFile(kfDef)
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: fmt.Sprintf("Error creating KfApp from config file: %v", err),
}
}
}

// Set default TypeMeta information. This will get overwritten by explicit values if set in the cfg file.
kfdef, err := kfdefsv3.LoadKFDefFromURI(cfgfile)
kfdef, err := kfdefsv3.LoadKFDefFromURI(appFile)
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INTERNAL_ERROR),
Expand All @@ -832,6 +759,7 @@ func LoadKfAppCfgFile(cfgfile string) (kftypesv3.KfApp, error) {
PackageManagers: make(map[string]kftypesv3.KfApp),
KfDef: kfdef,
}

// fetch the platform [gcp,minikube]
platform := c.KfDef.Spec.Platform
if platform != "" {
Expand All @@ -858,6 +786,28 @@ func LoadKfAppCfgFile(cfgfile string) (kftypesv3.KfApp, error) {
}
}

// If the config file is downloaded remotely, use the current working directory to create the KfApp.
// Otherwise use the directory where the config file is stored.
if isRemoteFile {
cwd, err = os.Getwd()
if err != nil {
return nil, &kfapis.KfError{
Code: int(kfapis.INTERNAL_ERROR),
Message: fmt.Sprintf("could not get current directory for KfDef %v", err),
}
}
c.KfDef.Spec.AppDir = cwd
} else {
c.KfDef.Spec.AppDir = path.Dir(cfgfile)
}

// Set some defaults
// TODO(jlewi): This code doesn't belong here. It should probably be called from inside KfApp; e.g. from
// KfApp.generate. We should do all initialization of defaults as part of the reconcile loop in one function.
if c.KfDef.Spec.PackageManager == "" {
c.KfDef.Spec.PackageManager = kftypesv3.KUSTOMIZE
}

return c, nil
}

Expand Down
6 changes: 3 additions & 3 deletions bootstrap/pkg/kfapp/gcp/gcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -2012,19 +2012,19 @@ func (gcp *Gcp) Generate(resources kftypesv3.ResourceEnum) error {
if gcp.kfDef.Spec.Project == "" {
return &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: "GCP Project is not set, please set it in KFDef.",
Message: "GCP Project is not set, please set it in KFDef or use gcloud config set project to set a default.",
}
}
if gcp.kfDef.Spec.Email == "" {
return &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: "GCP account could not be determined, please set Email in KFDef.",
Message: "GCP account could not be determined, please set Email in KFDef or use gcloud config set account to set a default.",
}
}
if gcp.kfDef.Spec.Zone == "" {
return &kfapis.KfError{
Code: int(kfapis.INVALID_ARGUMENT),
Message: "GCP Zone is not set, please set it in KFDef.",
Message: "GCP Zone is not set, please set it in KFDef or use gcloud config set compute/zone to set a default.",
}
}
// Set default IPName and Hostname
Expand Down
19 changes: 12 additions & 7 deletions testing/kfctl/kf_is_ready_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,18 +69,23 @@ def test_kf_is_ready(namespace, use_basic_auth, use_istio):
logging.info("Verifying that deployment %s started...", deployment_name)
util.wait_for_deployment(api_client, namespace, deployment_name, 10)

for stateful_set_name in stateful_set_names:
logging.info("Verifying that stateful set %s started...", stateful_set_name)
util.wait_for_statefulset(api_client, namespace, stateful_set_name)

ingress_namespace = "istio-system" if use_istio else namespace
for deployment_name in ingress_related_deployments:
logging.info("Verifying that deployment %s started...", deployment_name)
util.wait_for_deployment(api_client, ingress_namespace, deployment_name, 10)

for name in ingress_related_stateful_sets:
logging.info("Verifying that statefulset %s started...", name)
util.wait_for_statefulset(api_client, ingress_namespace, name)

all_stateful_sets = [(namespace, name) for name in stateful_set_names]
all_stateful_sets.extend([(ingress_namespace, name) for name in ingress_related_stateful_sets])

for ss_namespace, name in all_stateful_sets:
logging.info("Verifying that stateful set %s.%s started...", ss_namespace, name)
try:
util.wait_for_statefulset(api_client, ss_namespace, name)
except:
# Collect debug information by running describe
util.run(["kubectl", "-n", ss_namespace, "describe", "statefulsets", name])
raise

# TODO(jlewi): We should verify that the ingress is created and healthy.

Expand Down

0 comments on commit 3b27111

Please sign in to comment.