Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unstick helm commands by removing helm secrets #154

Merged
merged 2 commits into from
Dec 13, 2024
Merged

fix: unstick helm commands by removing helm secrets #154

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11f4e49
remove helm secrets if previous attempt is stuck
colesnodgrass Dec 13, 2024
72f6923
handle all-attempts-fail situation
colesnodgrass Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 18 additions & 21 deletions internal/cmd/local/k8s/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,50 +33,40 @@ type Client interface {
// This is a blocking call, it should only return once the deployment has completed.
DeploymentRestart(ctx context.Context, namespace, name string) error

// IngressCreate creates an ingress in the given namespace
EventsWatch(ctx context.Context, namespace string) (watch.Interface, error)

IngressCreate(ctx context.Context, namespace string, ingress *networkingv1.Ingress) error
// IngressExists returns true if the ingress exists in the namespace, false otherwise.
IngressExists(ctx context.Context, namespace string, ingress string) bool
// IngressUpdate updates an existing ingress in the given namespace
IngressUpdate(ctx context.Context, namespace string, ingress *networkingv1.Ingress) error

// NamespaceCreate creates a namespace
LogsGet(ctx context.Context, namespace string, name string) (string, error)

NamespaceCreate(ctx context.Context, namespace string) error
// NamespaceExists returns true if the namespace exists, false otherwise
NamespaceExists(ctx context.Context, namespace string) bool
// NamespaceDelete deletes the existing namespace
NamespaceDelete(ctx context.Context, namespace string) error

// PersistentVolumeCreate creates a persistent volume
PersistentVolumeCreate(ctx context.Context, namespace, name string) error
// PersistentVolumeExists returns true if the persistent volume exists, false otherwise
PersistentVolumeExists(ctx context.Context, namespace, name string) bool
// PersistentVolumeDelete deletes the existing persistent volume
PersistentVolumeDelete(ctx context.Context, namespace, name string) error

// PersistentVolumeClaimCreate creates a persistent volume claim
PersistentVolumeClaimCreate(ctx context.Context, namespace, name, volumeName string) error
// PersistentVolumeClaimExists returns true if the persistent volume claim exists, false otherwise
PersistentVolumeClaimExists(ctx context.Context, namespace, name, volumeName string) bool
// PersistentVolumeClaimDelete deletes the existing persistent volume claim
PersistentVolumeClaimDelete(ctx context.Context, namespace, name, volumeName string) error

// SecretCreateOrUpdate will update or create the secret name with the payload of data in the specified namespace
PodList(ctx context.Context, namespace string) (*corev1.PodList, error)

SecretCreateOrUpdate(ctx context.Context, secret corev1.Secret) error
// SecretGet returns the secrets for the namespace and name
// SecretDeleteCollection deletes multiple secrets.
// Note this takes a `type` and not a `name`. All secrets matching this type will be removed.
SecretDeleteCollection(ctx context.Context, namespace, _type string) error
SecretGet(ctx context.Context, namespace, name string) (*corev1.Secret, error)

// ServiceGet returns the service for the given namespace and name
ServiceGet(ctx context.Context, namespace, name string) (*corev1.Service, error)

StreamPodLogs(ctx context.Context, namespace string, podName string, since time.Time) (io.ReadCloser, error)

// ServerVersionGet returns the kubernetes version.
ServerVersionGet() (string, error)

EventsWatch(ctx context.Context, namespace string) (watch.Interface, error)

LogsGet(ctx context.Context, namespace string, name string) (string, error)
StreamPodLogs(ctx context.Context, namespace string, podName string, since time.Time) (io.ReadCloser, error)
PodList(ctx context.Context, namespace string) (*corev1.PodList, error)
}

var _ Client = (*DefaultK8sClient)(nil)
Expand Down Expand Up @@ -289,6 +279,13 @@ func (d *DefaultK8sClient) SecretCreateOrUpdate(ctx context.Context, secret core
return fmt.Errorf("unexpected error while handling the secret %s: %w", name, err)
}

func (d *DefaultK8sClient) SecretDeleteCollection(ctx context.Context, namespace, _type string) error {
listOptions := metav1.ListOptions{
FieldSelector: "type=" + _type,
}
return d.ClientSet.CoreV1().Secrets(namespace).DeleteCollection(ctx, metav1.DeleteOptions{}, listOptions)
}

func (d *DefaultK8sClient) SecretGet(ctx context.Context, namespace, name string) (*corev1.Secret, error) {
secret, err := d.ClientSet.CoreV1().Secrets(namespace).Get(ctx, name, metav1.GetOptions{})
if err != nil {
Expand Down
9 changes: 9 additions & 0 deletions internal/cmd/local/k8s/k8stest/k8stest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type MockClient struct {
FnPersistentVolumeClaimExists func(ctx context.Context, namespace, name, volumeName string) bool
FnPersistentVolumeClaimDelete func(ctx context.Context, namespace, name, volumeName string) error
FnSecretCreateOrUpdate func(ctx context.Context, secret corev1.Secret) error
FnSecretDeleteCollection func(ctx context.Context, namespace, _type string) error
FnSecretGet func(ctx context.Context, namespace, name string) (*corev1.Secret, error)
FnServerVersionGet func() (string, error)
FnServiceGet func(ctx context.Context, namespace, name string) (*corev1.Service, error)
Expand Down Expand Up @@ -146,6 +147,14 @@ func (m *MockClient) SecretGet(ctx context.Context, namespace, name string) (*co
return nil, nil
}

func (m *MockClient) SecretDeleteCollection(ctx context.Context, namespace, _type string) error {
if m.FnSecretDeleteCollection != nil {
return m.FnSecretDeleteCollection(ctx, namespace, _type)
}

return nil
}

func (m *MockClient) ServiceGet(ctx context.Context, namespace, name string) (*corev1.Service, error) {
return m.FnServiceGet(ctx, namespace, name)
}
Expand Down
61 changes: 38 additions & 23 deletions internal/cmd/local/local/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,29 +648,44 @@ func (c *Command) handleChart(
}
}

pterm.Info.Println(fmt.Sprintf(
"Starting Helm Chart installation of '%s' (version: %s)",
req.chartName, helmChart.Metadata.Version,
))
c.spinner.UpdateText(fmt.Sprintf(
"Installing '%s' (version: %s) Helm Chart (this may take several minutes)",
req.chartName, helmChart.Metadata.Version,
))
helmRelease, err := c.helm.InstallOrUpgradeChart(ctx, &helmclient.ChartSpec{
ReleaseName: req.chartRelease,
ChartName: req.chartLoc,
CreateNamespace: true,
Namespace: req.namespace,
Wait: true,
Timeout: 60 * time.Minute,
ValuesYaml: req.valuesYAML,
Version: req.chartVersion,
},
&helmclient.GenericHelmOptions{},
)
if err != nil {
pterm.Error.Printfln("Failed to install %s Helm Chart", req.chartName)
return fmt.Errorf("unable to install helm: %w", err)
var helmRelease *release.Release

// it's possible that an existing helm installation is stuck in a non-final state
// which this code will detect, attempt to clean up, and try again up to three times
for attemptCount := 0; attemptCount < 3; attemptCount++ {
pterm.Info.Println(fmt.Sprintf(
"Starting Helm Chart installation of '%s' (version: %s)",
req.chartName, helmChart.Metadata.Version,
))
c.spinner.UpdateText(fmt.Sprintf(
"Installing '%s' (version: %s) Helm Chart (this may take several minutes)",
req.chartName, helmChart.Metadata.Version,
))

helmRelease, err = c.helm.InstallOrUpgradeChart(ctx, &helmclient.ChartSpec{
ReleaseName: req.chartRelease,
ChartName: req.chartLoc,
CreateNamespace: true,
Namespace: req.namespace,
Wait: true,
Timeout: 60 * time.Minute,
ValuesYaml: req.valuesYAML,
Version: req.chartVersion,
},
&helmclient.GenericHelmOptions{},
)

if err != nil {
if strings.Contains(err.Error(), "another operation (install/upgrade/rollback) is in progress") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to check attempt count somewhere? I think you could check it here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't need to check it here, but I need to check if it was successful outside of this loop.

Copy link
Member Author

@colesnodgrass colesnodgrass Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abuchanan-airbyte I added a check after the for-loop to ensure that helmRelease was properly set (and a new unit-test as well). I don't care what the attemptCount value is, I only care that we attempted more than once (after removing the helm-secrets on the first failed attempt) and less an an infinite amount. I picked three attempts as it was one more than the number of attempts it should realistically take (two).

Additionally I added a new localerr.ErrHelmStuck error that will now display information to the end-user on how to fix this problem if the newly added (in this PR) functionality to fix this problem for them fails.

if err := c.k8s.SecretDeleteCollection(ctx, common.AirbyteNamespace, "helm.sh/release.v1"); err != nil {
pterm.Debug.Println(fmt.Sprintf("unable to delete secrets helm.sh/release.v1: %s", err))
}
continue
}
pterm.Error.Printfln("Failed to install %s Helm Chart", req.chartName)
return fmt.Errorf("unable to install helm: %w", err)
}
break
}

c.tel.Attr(fmt.Sprintf("helm_%s_release_version", req.name), strconv.Itoa(helmRelease.Version))
Expand Down
154 changes: 154 additions & 0 deletions internal/cmd/local/local/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,159 @@ func TestCommand_Install(t *testing.T) {
}
}

func TestCommand_InstallBadHelmState(t *testing.T) {
valuesYaml := mustReadFile(t, "testdata/test-edition.values.yaml")
expChartRepoCnt := 0
expChartRepo := []struct {
name string
url string
}{
{name: common.AirbyteRepoName, url: common.AirbyteRepoURL},
{name: common.NginxRepoName, url: common.NginxRepoURL},
}

expChartCnt := 0
expNginxValues, _ := helm.BuildNginxValues(9999)
expChart := []struct {
chart helmclient.ChartSpec
release release.Release
}{
{
chart: helmclient.ChartSpec{
ReleaseName: common.AirbyteChartRelease,
ChartName: testAirbyteChartLoc,
Namespace: common.AirbyteNamespace,
CreateNamespace: true,
Wait: true,
Timeout: 60 * time.Minute,
ValuesYaml: valuesYaml,
},
release: release.Release{
Chart: &chart.Chart{Metadata: &chart.Metadata{Version: "1.2.3.4"}},
Name: common.AirbyteChartRelease,
Namespace: common.AirbyteNamespace,
Version: 0,
},
},
{
chart: helmclient.ChartSpec{
ReleaseName: common.NginxChartRelease,
ChartName: common.NginxChartName,
Namespace: common.NginxNamespace,
CreateNamespace: true,
Wait: true,
Timeout: 60 * time.Minute,
ValuesYaml: expNginxValues,
},
release: release.Release{
Chart: &chart.Chart{Metadata: &chart.Metadata{Version: "4.3.2.1"}},
Name: common.NginxChartRelease,
Namespace: common.NginxNamespace,
Version: 0,
},
},
}

installCalled := false
helm := mockHelmClient{
addOrUpdateChartRepo: func(entry repo.Entry) error {
if d := cmp.Diff(expChartRepo[expChartRepoCnt].name, entry.Name); d != "" {
t.Error("chart name mismatch", d)
}
if d := cmp.Diff(expChartRepo[expChartRepoCnt].url, entry.URL); d != "" {
t.Error("chart url mismatch", d)
}

expChartRepoCnt++

return nil
},

getChart: func(name string, _ *action.ChartPathOptions) (*chart.Chart, string, error) {
switch {
case name == testAirbyteChartLoc:
return &chart.Chart{Metadata: &chart.Metadata{Version: "test.airbyte.version"}}, "", nil
case name == common.NginxChartName:
return &chart.Chart{Metadata: &chart.Metadata{Version: "test.nginx.version"}}, "", nil
default:
t.Error("unsupported chart name", name)
return nil, "", errors.New("unexpected chart name")
}
},

getRelease: func(name string) (*release.Release, error) {
switch {
case name == common.AirbyteChartRelease:
t.Error("should not have been called", name)
return nil, errors.New("should not have been called")
case name == common.NginxChartRelease:
return nil, errors.New("not found")
default:
t.Error("unsupported chart name", name)
return nil, errors.New("unexpected chart name")
}
},

installOrUpgradeChart: func(ctx context.Context, spec *helmclient.ChartSpec, opts *helmclient.GenericHelmOptions) (*release.Release, error) {
if d := cmp.Diff(&expChart[expChartCnt].chart, spec); d != "" {
t.Error("chart mismatch", d)
}

if installCalled {
defer func() { expChartCnt++ }()

return &expChart[expChartCnt].release, nil
}

installCalled = true
return nil, errors.New("another operation (install/upgrade/rollback) is in progress")
},

uninstallReleaseByName: func(s string) error {
if d := cmp.Diff(expChart[expChartCnt].release.Name, s); d != "" {
t.Error("release mismatch", d)
}

return nil
},
}

k8sClient := k8stest.MockClient{
FnIngressExists: func(ctx context.Context, namespace string, ingress string) bool {
return false
},
}

tel := telemetry.MockClient{}

httpClient := mockHTTP{do: func(req *http.Request) (*http.Response, error) {
return &http.Response{StatusCode: 200}, nil
}}

c, err := New(
k8s.TestProvider,
WithPortHTTP(portTest),
WithHelmClient(&helm),
WithK8sClient(&k8sClient),
WithTelemetryClient(&tel),
WithHTTPClient(&httpClient),
WithBrowserLauncher(func(url string) error {
return nil
}),
)
if err != nil {
t.Fatal(err)
}

installOpts := &InstallOpts{
HelmValuesYaml: valuesYaml,
AirbyteChartLoc: testAirbyteChartLoc,
}
if err := c.Install(context.Background(), installOpts); err != nil {
t.Fatal(err)
}
}

func TestCommand_InstallError(t *testing.T) {
testErr := errors.New("test error")
valuesYaml := mustReadFile(t, "testdata/test-edition.values.yaml")
Expand Down Expand Up @@ -234,6 +387,7 @@ func TestCommand_InstallError(t *testing.T) {
}

func mustReadFile(t *testing.T, name string) string {
t.Helper()
b, err := os.ReadFile(name)
if err != nil {
t.Fatal(err)
Expand Down
Loading