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
Reconcile parent instances when a child instance is done #1568
Conversation
Summary: after updating the [flink-demo](kudobuilder/operators#279) to use dependencies a few fixes were needed: - instance controller now properly reconciles (parent) instances owning child instances - `InClusterResolver` ignores operators `appVersion` if it wasn't set - child instance names are generated differently: - if the `instanceName` was set, it is taken without modifications - otherwise, `parentInstance-childOperator` is taken e.g. `kafka-zookeeper`. in this case we also use `-` instead of `.` separator since instance names are often used as service names (DNS-1035 label) which only allow alphanumeric characters or `-` in them. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the core change lgtm
It would be great to have clarification around comments of controller registration to the manager
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
…e operator was resolved Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
@@ -25,11 +25,6 @@ func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersi | |||
return nil, fmt.Errorf("failed to resolve operator version %s/%s:%s", r.ns, ovn, appVersion) | |||
} | |||
|
|||
// sanity check, as there is an explicit 1:1 relationship between an operator and app version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the second thought: this sanity check didn't make any sense
@@ -143,6 +143,7 @@ func updateKudoOperatorTaskPackageNames( | |||
for _, dependency := range dependencies { | |||
if tasks[i].Spec.KudoOperatorTaskSpec.Package == dependency.PackageName { | |||
tasks[i].Spec.KudoOperatorTaskSpec.Package = dependency.Operator.Name | |||
tasks[i].Spec.KudoOperatorTaskSpec.OperatorVersion = dependency.OperatorVersion.Spec.Version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nfnt this is for cases when the user didn't specify the KudoOperatorTaskSpec.OperatorVersion
and it was resolved to the latest one. the field has to be populated by the CLI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I wasn't aware of the implications of this field being optional. In this case, the same must be done for KudoOperatorTaskSpec.AppVersion
. And the function name and description needs to be updated as well, how about simply updateKudoOperatorTask
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree about the method name but the AppVersion
is not needed on the server-side so 🤷
Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Summary:
after updating the flink-demo to use dependencies a few fixes were needed:
InClusterResolver
ignores operatorsappVersion
if it wasn't setinstanceName
was set, it is taken without modificationsparentInstance-childOperator
is taken e.g.kafka-zookeeper
. in this case, we also use-
instead of.
separator since instance names are often used as service names (DNS-1035 label) which only allow alphanumeric characters or-
in them.Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com