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

KEP-29: Introducing operator dependency cycle detection to the instance controller #1559

Merged
merged 5 commits into from Jun 22, 2020

Conversation

zen-dog
Copy link
Contributor

@zen-dog zen-dog commented Jun 11, 2020

Summary:
Operator dependencies cycle detection is now part of the instance controller reconciliation. We execute the check for the top-level instances only since the lower-level instances are already taken care of.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

…oller

Summary:
Operator dependencies cycle detection is now part of the instance controller reconciliation. We execute the check for the top-level instances only, since the lower-level instances are already take care of.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@@ -222,14 +221,3 @@ type InstanceList struct {
func init() {
SchemeBuilder.Register(&Instance{}, &InstanceList{})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used

// In (1), i.Spec.PlanExecution.PlanName is set directly when the user updates the instance and reset **after** the plan
// is terminal
// In (2) i.Spec.PlanStatus is updated **AFTER** the instance controller is done with the reconciliation call
func (i *Instance) GetScheduledPlan() *PlanStatus {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used

@zen-dog zen-dog changed the title Introducing operator dependency cycle detection to the instance controller KEP-29: Introducing operator dependency cycle detection to the instance controller Jun 12, 2020
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

LGTM overall. But needs tests to ensure that changes to the dependency checker won't break the new package resolver.

@@ -50,6 +50,8 @@ import (
"github.com/kudobuilder/kudo/pkg/engine/renderer"
"github.com/kudobuilder/kudo/pkg/engine/task"
"github.com/kudobuilder/kudo/pkg/engine/workflow"
"github.com/kudobuilder/kudo/pkg/kudoctl/packages"
Copy link
Member

Choose a reason for hiding this comment

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

With package handling no longer exclusive to the CLI, let's move the packages folder up in the package hierarchy. pkg/kudoctl/packages -> pkg/packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind but let's do it in a separate PR

Comment on lines 39 to 45
return &packages.Package{
Resources: &packages.Resources{
Operator: o,
OperatorVersion: ov,
Instance: nil,
},
Files: nil,
Copy link
Member

Choose a reason for hiding this comment

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

This is making the assumption that the dependency resolver will only parse the OperatorVersion of a package -- which it currently does. We need to make sure that this assumption is covered by a test that would fail if, e.g., the dependency resolver starts to parse Instance as well.

pkg/apis/kudo/v1beta1/operator_types_helpers.go Outdated Show resolved Hide resolved
pkg/apis/kudo/v1beta1/operatorversion_types_helpers.go Outdated Show resolved Hide resolved
pkg/controller/instance/resolver_online.go Outdated Show resolved Hide resolved
additionally removed `KudoOperatorTask.OperatorName` since we don't need it anymore. Fixed a few other issue too.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Copy link
Member

@nfnt nfnt left a comment

Choose a reason for hiding this comment

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

This is looking great!
I'm not sure about the kind comparison in IsChildInstance though. Please add a comment explaining why this works.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@zen-dog zen-dog merged commit d01b859 into master Jun 22, 2020
@zen-dog zen-dog deleted the ad/ic-dependency-cycles branch June 22, 2020 09:49
zen-dog pushed a commit that referenced this pull request Jun 22, 2020
…ce controller (#1559)

Summary:
Operator dependencies cycle detection is now part of the instance controller reconciliation. We execute the check for the top-level instances only since the lower-level instances are already take care of.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
(cherry picked from commit d01b859)
zen-dog pushed a commit that referenced this pull request Jun 22, 2020
…ce controller (#1559) (#1567)

Summary:
Operator dependencies cycle detection is now part of the instance controller reconciliation. We execute the check for the top-level instances only since the lower-level instances are already take care of.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Fixes #1513
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants