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.

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

chore(feature-tracker): simplifies struct handling #1092

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 1 addition & 1 deletion pkg/feature/feature.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
if err != nil {
return err
}
errGet := getFeatureTrackerIfAbsent(ctx, f)

Check failure on line 65 in pkg/feature/feature.go

View workflow job for this annotation

GitHub Actions / unit-test

undefined: getFeatureTrackerIfAbsent

Check failure on line 65 in pkg/feature/feature.go

View workflow job for this annotation

GitHub Actions / golangci-lint

undefined: getFeatureTrackerIfAbsent (typecheck)

Check failure on line 65 in pkg/feature/feature.go

View workflow job for this annotation

GitHub Actions / golangci-lint

undefined: getFeatureTrackerIfAbsent) (typecheck)

Check failure on line 65 in pkg/feature/feature.go

View workflow job for this annotation

GitHub Actions / golangci-lint

undefined: getFeatureTrackerIfAbsent) (typecheck)

Check failure on line 65 in pkg/feature/feature.go

View workflow job for this annotation

GitHub Actions / golangci-lint

undefined: getFeatureTrackerIfAbsent) (typecheck)
if errGet == nil {
return f.Cleanup(ctx)
}
Expand All @@ -70,7 +70,7 @@
return client.IgnoreNotFound(errGet)
}

if trackerErr := f.createFeatureTracker(ctx); trackerErr != nil {
if trackerErr := createFeatureTracker(ctx, f); trackerErr != nil {
return trackerErr
}

Expand Down
69 changes: 36 additions & 33 deletions pkg/feature/feature_tracker_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,26 @@ func (e *withConditionReasonError) Error() string {
return e.err.Error()
}

// createFeatureTracker instantiates FeatureTracker for a given Feature. It's a cluster-scoped resource used
// to track creation and removal of all owned resources which belong to this Feature.
// All resources which particular feature is composed of will have this object attached as an OwnerReference.
func (f *Feature) createFeatureTracker(ctx context.Context) error {
tracker, err := f.getFeatureTracker(ctx)
if k8serr.IsNotFound(err) {
if err := f.Client.Create(ctx, tracker); err != nil {
return err
// createFeatureTracker creates a FeatureTracker, persists it in the cluster,
// and attaches it to the provided Feature instance.
func createFeatureTracker(ctx context.Context, f *Feature) error {
tracker, errGet := getFeatureTracker(ctx, f)
if client.IgnoreNotFound(errGet) != nil {
return errGet
}

if k8serr.IsNotFound(errGet) {
tracker = featurev1.NewFeatureTracker(f.Name, f.Spec.AppNamespace)
tracker.Spec = featurev1.FeatureTrackerSpec{
Source: *f.Spec.Source,
AppNamespace: f.Spec.AppNamespace,
}
if errCreate := f.Client.Create(ctx, tracker); errCreate != nil {
return errCreate
}
} else if err != nil {
return err
}

if gvkErr := f.ensureGVKSet(tracker); gvkErr != nil {
if gvkErr := ensureGVKSet(tracker, f.Client.Scheme()); gvkErr != nil {
return gvkErr
}

Expand All @@ -50,39 +56,36 @@ func (f *Feature) createFeatureTracker(ctx context.Context) error {
}

func removeFeatureTracker(ctx context.Context, f *Feature) error {
if err := getFeatureTrackerIfAbsent(ctx, f); err != nil {
return client.IgnoreNotFound(err)
associatedTracker := f.Tracker
if associatedTracker == nil {
// Check if it is persisted in the cluster, but Feature do not have it attached
if tracker, errGet := getFeatureTracker(ctx, f); client.IgnoreNotFound(errGet) != nil {
return errGet
} else {
associatedTracker = tracker
Copy link
Member

Choose a reason for hiding this comment

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

is there any possibility that after associatedTracker = tracker associatedTracker is nil?
otherwise why not just
return client.IgnoreNotFound(f.Client.Delete(ctx, tracker))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, it can be not found in L62, and so it is nil. We do not propagate error on 404 (client.IgnoreNotFound(errGet)), but we handle this case by simply skipping the Delete operation. Calling Delete on nil will panic.

As to part if this can happen at runtime - under the hood it boils down to which operations are performed directly against kube-apiserver and which are taken from the local cache. I think it can be stale and might lead to such situation, so I thought it's better to handle it explicitly.

}
}

if associatedTracker != nil {
return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker))
}

return deleteTracker(ctx, f)
return nil
}

func (f *Feature) getFeatureTracker(ctx context.Context) (*featurev1.FeatureTracker, error) {
func getFeatureTracker(ctx context.Context, f *Feature) (*featurev1.FeatureTracker, error) {
tracker := featurev1.NewFeatureTracker(f.Name, f.Spec.AppNamespace)

tracker.Spec = featurev1.FeatureTrackerSpec{
Source: *f.Spec.Source,
AppNamespace: f.Spec.AppNamespace,
if errGet := f.Client.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil {
return nil, errGet
}

err := f.Client.Get(ctx, client.ObjectKeyFromObject(tracker), tracker)

return tracker, err
}

func deleteTracker(ctx context.Context, f *Feature) error {
return client.IgnoreNotFound(f.Client.Delete(ctx, f.Tracker))
}

func getFeatureTrackerIfAbsent(ctx context.Context, f *Feature) error {
var err error
f.Tracker, err = f.getFeatureTracker(ctx)
return err
return tracker, nil
}

func (f *Feature) ensureGVKSet(obj runtime.Object) error {
func ensureGVKSet(obj runtime.Object, scheme *runtime.Scheme) error {
// See https://github.com/kubernetes/client-go/issues/308
gvks, unversioned, err := f.Client.Scheme().ObjectKinds(obj)
gvks, unversioned, err := scheme.ObjectKinds(obj)
if err != nil {
return fmt.Errorf("failed to get group, version, & kinds for object: %w", err)
}
Expand Down
Loading