Add node controller to HCCO Manager #1702

enxebre · 2022-08-25T12:17:44Z

What this PR does / why we need it:
What:
This introduces a feature to reconcile Nodes with an opinionated Hypershift label pointing to the owning NodePool.

Why:
This is useful to list and filter Nodes by NodePool. This is useful for all operators to have a single and consistent bidirectional way for NodePool<->Node without having to reimplement the same logic, vendoring clients and consuming compute resources. E.g NTO tuneD.

How:
Change the NodePool controller to propagate the NodePool annotation down to Machines.
Introduce a Node controller within the HCCO manager:

Watches Nodes.
Find the Machine by using the appropriate CAPI annotations.
Find the NodePool annotation.
Apply the annotation as a Label into the Node.

In future we might extend this to support label propagation into Nodes via NodePool API.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
ref https://issues.redhat.com/browse/HOSTEDCP-545

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

enxebre · 2022-08-25T12:17:59Z

/hold
to do some additional manual test.

enxebre · 2022-08-25T12:19:08Z

cc @dagrayvid

openshift-ci · 2022-08-25T12:23:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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:

~~OWNERS~~ [enxebre]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dagrayvid · 2022-08-25T21:34:15Z

@enxebre, I don't think we can label nodes with the full / of the NodePools, since '/' characters aren't allowed for labels, for instance:

oc label node/ip-10-0-134-129.ec2.internal "hypershift.openshift.io/nodePool"="namespace/name"
error: invalid label value: "hypershift.openshift.io/nodePool=namespace/name": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')

Unless I'm missing something, this doesn't account for that yet. Will we need 2 separate labels, one for namespace and one for name?

enxebre · 2022-08-26T07:33:47Z

Unless I'm missing something, this doesn't account for that yet. Will we need 2 separate labels, one for namespace and one for name?

Only the name is needed to filter Nodes, updated.

enxebre · 2022-08-26T13:23:22Z

/hold cancel
/test e2e-aws-nested

dagrayvid · 2022-08-26T13:43:53Z

@enxebre for NTO we have 2 uses for these labels:

When choosing the TuneD profile for each Node, only consider TuneD profiles from the Tuned objects referenced in the Nodes NodePool spec. (Compare NodePool label on the Node with labels on the Tuned objects)
When generating MachineConfigs to pass back to the NodePool controller for setting kernel boot parameters (via ConfigMap), label the ConfigMap with the NodePool name (and namespace?) that the MachineConfig should be applied to.

In this second case, I think we likely need the name and the namespace, to make sure NTO and the NodePool controller are distinguishing between NodePools of the same name from separate namespaces.

Perhaps we could separate name and namespace by '_', as this is supported in label values but not allowed in object names? Or we could have two labels, one for name and one for namespace of the NodePool.

/cc @csrwng as this is related to the comment thread here and I would probably like to use the same convention if we end up going one of these routes.

enxebre · 2022-08-26T14:14:32Z

When generating MachineConfigs to pass back to the NodePool controller for setting kernel boot parameters (via ConfigMap), label the ConfigMap with the NodePool name (and namespace?) that the MachineConfig should be applied to.

The namespace is not needed, the relation goes in the other direction, the NodePool controller is aware of the cp namespace where its config live for each NodePool (in this case the config auto generated by the NTO).

dagrayvid · 2022-08-26T14:22:10Z

The namespace is not needed, the relation goes in the other direction, the NodePool controller is aware of the cp namespace where its config live for each NodePool (in this case the config auto generated by the NTO).

Ack, that makes sense. As long as it is not supported to have two NodePools of the same name in different namespaces for the same hosted cluster, we should be fine with just the name

enxebre · 2022-08-29T08:50:33Z

/test e2e-aws-nested

csrwng

Thanks @enxebre. Some comments.

csrwng · 2022-08-29T13:20:05Z

control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go

+)
+
+const (
+	nodePoolAnnotation = "hypershift.openshift.io/nodePool"


s/nodePoolAnnotation/nodePoolLabel/

should this be part of the API?

It's both annotation and label. I updated to move the label declaration into API.

csrwng · 2022-08-29T13:21:38Z

control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go

+	var apiErr *apierrors.StatusError
+	nodePoolName, err := r.nodeToNodePoolName(node)
+	if err != nil {
+		if errors.As(err, &apiErr) && !apierrors.IsNotFound(err) {


s/IsNotFound(err)/IsNotFound(apiErr)/

I think is right as it is, isn't it? I want to pass the err I got through apierrors.IsNotFound check.

but the check would only work on StatusError no? (otherwise I don't see the point of checking that err contains a StatusError)

csrwng · 2022-08-29T13:22:51Z

control-plane-operator/hostedclusterconfigoperator/controllers/node/node.go

+			// annotations are not in place yet, so we'll reconcile triggered by the event which sets them in the Node.
+			return ctrl.Result{}, err
+		} else {
+			log.Error(err, "failed to get nodePool name from Node")


add node name to error args

it should come with logger as it's part of the request.

csrwng · 2022-08-29T13:28:06Z

control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go

+			error:                true,
+		},
+		{
+			name:                 "When Machine does not exist it should fail",


Seems that this describes the case above, this case looks like "When node has no annotations"

csrwng · 2022-08-29T13:30:24Z

control-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.go

+			},
+		},
+	}
+	machineWithOutNodePoolAnnotation := &capiv1.Machine{


This doesn't seem to be used in any of the tests.

csrwng · 2022-08-29T13:37:42Z

test/e2e/create_cluster_test.go

+		t.Fatalf("failed to list nodes in guest cluster: %v", err)
+	}
+	for _, node := range nodes.Items {
+		g.Expect(node.Labels["hypershift.openshift.io/nodePool"]).NotTo(BeEmpty())


Use constant (esp if you move it to the API)

What: This introduces a feature to reconcile Nodes with an opinionated Hypershift label pointing to the owning NodePool. Why: This is useful to list and filter Nodes by NodePool. This is useful for all operators to have a single and consistent bidirectional way for NodePool<->Node without having to reimplement the same logic, vendoring clients and consuming compute resources. E.g NTO tuneD. How: Change the NodePool controller to propagate the NodePool annotation down to Machines. Introduce a Node controller within the HCCO manager: - Watches Nodes. - Find the Machine by using the appropriate CAPI annotations. - Find the NodePool annotation. - Apply the annotation as a Label into the Node. In future we might extend this to support label propagation into Nodes via NodePool API.

netlify · 2022-08-29T15:04:39Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`6e484d5`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/630cd53246343f0008fe7a31
😎 Deploy Preview	https://deploy-preview-1702--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

csrwng · 2022-08-29T17:53:32Z

/lgtm

Looks like we're hitting AWS limits:

NatGatewayLimitExceeded: Performing this operation would exceed the limit of 80 NAT gateways

openshift-ci-robot · 2022-08-29T18:35:53Z

/retest-required

Remaining retests: 2 against base HEAD b242dee and 8 for PR HEAD 6e484d5 in total

openshift-ci · 2022-08-30T02:52:13Z

@enxebre: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/capi-provider-agent-sanity	`6e484d5`	link	false	`/test capi-provider-agent-sanity`
ci/prow/e2e-aws-nested	`c7b8dd9`	link	true	`/test e2e-aws-nested`
ci/prow/e2e-kubevirt-gcp-ovn	`6e484d5`	link	false	`/test e2e-kubevirt-gcp-ovn`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 25, 2022

openshift-ci bot requested review from csrwng and sjenning August 25, 2022 12:23

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2022

enxebre force-pushed the propagate-nodepool-label branch 4 times, most recently from 9a0ef0e to 37399a9 Compare August 25, 2022 14:58

dagrayvid mentioned this pull request Aug 26, 2022

Enable Node Tuning Operator in HyperShift #1651

Merged

4 tasks

enxebre force-pushed the propagate-nodepool-label branch from 37399a9 to c7b8dd9 Compare August 26, 2022 07:32

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 26, 2022

csrwng reviewed Aug 29, 2022

View reviewed changes

enxebre force-pushed the propagate-nodepool-label branch from c7b8dd9 to 6e484d5 Compare August 29, 2022 15:03

openshift-ci bot assigned csrwng Aug 29, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2022

openshift-merge-robot merged commit 82ade29 into openshift:main Aug 30, 2022

dagrayvid mentioned this pull request Aug 31, 2022

Remove dynamic client and simplify getNodePoolNameFromNode openshift/cluster-node-tuning-operator#452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node controller to HCCO Manager #1702

Add node controller to HCCO Manager #1702

enxebre commented Aug 25, 2022 •

edited

enxebre commented Aug 25, 2022

enxebre commented Aug 25, 2022

openshift-ci bot commented Aug 25, 2022

dagrayvid commented Aug 25, 2022

enxebre commented Aug 26, 2022

enxebre commented Aug 26, 2022

dagrayvid commented Aug 26, 2022 •

edited

enxebre commented Aug 26, 2022

dagrayvid commented Aug 26, 2022

enxebre commented Aug 29, 2022

csrwng left a comment

csrwng Aug 29, 2022

csrwng Aug 29, 2022

enxebre Aug 29, 2022

csrwng Aug 29, 2022

enxebre Aug 29, 2022

csrwng Aug 29, 2022

csrwng Aug 29, 2022

enxebre Aug 29, 2022

csrwng Aug 29, 2022

enxebre Aug 29, 2022

csrwng Aug 29, 2022

enxebre Aug 29, 2022

csrwng Aug 29, 2022

netlify bot commented Aug 29, 2022

csrwng commented Aug 29, 2022

openshift-ci-robot commented Aug 29, 2022

openshift-ci bot commented Aug 30, 2022

Add node controller to HCCO Manager #1702

Add node controller to HCCO Manager #1702

Conversation

enxebre commented Aug 25, 2022 • edited

enxebre commented Aug 25, 2022

enxebre commented Aug 25, 2022

openshift-ci bot commented Aug 25, 2022

dagrayvid commented Aug 25, 2022

enxebre commented Aug 26, 2022

enxebre commented Aug 26, 2022

dagrayvid commented Aug 26, 2022 • edited

enxebre commented Aug 26, 2022

dagrayvid commented Aug 26, 2022

enxebre commented Aug 29, 2022

csrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Aug 29, 2022

✅ Deploy Preview for hypershift-docs ready!

csrwng commented Aug 29, 2022

openshift-ci-robot commented Aug 29, 2022

openshift-ci bot commented Aug 30, 2022

enxebre commented Aug 25, 2022 •

edited

dagrayvid commented Aug 26, 2022 •

edited