fixes 'kfctl apply fails to create k8s resources; tries to deploy to 127.0.0.1' #2813
Conversation
|
/assign @jlewi |
|
/assign @kunmingg |
|
Can you remove the retry logic around kubeflow/testing/kfctl/kfctl_go_test.py Line 79 in 4485a65 Right now that retry logic is masking problems like this. If there are legitimate retryable failures we can bad more specific retry logic that only retries when those specific errors occur. |
| @@ -437,10 +437,16 @@ func (ksApp *ksApp) envSet(envName string, host string) error { | |||
| actions.OptionAppRoot: ksApp.ksRoot(), | |||
| actions.OptionEnvName: ksApp.KsEnvName, | |||
| actions.OptionServer: host, | |||
| actions.OptionOverride: true, | |||
There was a problem hiding this comment.
What's this doing? Is this just an idiosyncrancy of ksonnet that they require you set the optionOverride to explicitly tell it that the host associated with the environment should be used as opposed to the one set during init?
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("There was a problem setting ksonnet env: %v", err) | ||
| } | ||
| loadApp, loadErr := app.Load(afero.NewOsFs(), ksApp.KApp.HTTPClient(), ksApp.ksRoot()) |
There was a problem hiding this comment.
I see. So I'm guessing RunEnvSet is just modifying the config files on disks and we need to explicitly load the app to pick up the changes.
|
Looks good. Only comment is to update the test so that we can verify its fixed and catch errors like this in the future. |
There was a problem hiding this comment.
ok, updated to remove retry logic around kfctl apply.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ellis-bigelow, @jlewi, and @kunmingg)
bootstrap/pkg/kfapp/ksonnet/ksonnet.go, line 440 at r1 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
What's this doing? Is this just an idiosyncrancy of ksonnet that they require you set the optionOverride to explicitly tell it that the host associated with the environment should be used as opposed to the one set during init?
correct, subtle but critical
bootstrap/pkg/kfapp/ksonnet/ksonnet.go, line 445 at r1 (raw file):
Previously, jlewi (Jeremy Lewi) wrote…
I see. So I'm guessing RunEnvSet is just modifying the config files on disks and we need to explicitly load the app to pick up the changes.
also correct.
|
/lgtm |
|
Looks like a tf-serving flake |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
…127.0.0.1' (kubeflow#2813) * fixes 'kfctl apply fails to create k8s resources; tries to deploy to 127.0.0.1' * remove run_with_retries for kfctl_go_test kfctl apply use case
…127.0.0.1' (kubeflow#2813) * fixes 'kfctl apply fails to create k8s resources; tries to deploy to 127.0.0.1' * remove run_with_retries for kfctl_go_test kfctl apply use case
…127.0.0.1' (kubeflow#2813) * fixes 'kfctl apply fails to create k8s resources; tries to deploy to 127.0.0.1' * remove run_with_retries for kfctl_go_test kfctl apply use case
fixes #2791
This change is