OCPCLOUD-2493: Update CAO to add upstream annotations #316

racheljpg · 2024-03-04T17:00:12Z

Hello! This is a PR to update the CAO to add the upstream scale from zero annotations if it's only the old annotations that are there. Thanks!

openshift-ci-robot · 2024-03-04T17:00:17Z

@racheljpg: This pull request references OCPCLOUD-2493 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

Hello! This is a PR to update the CAO to add the upstream scale from zero annotations if it's only the old annotations that are there. Thanks!

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 openshift-eng/jira-lifecycle-plugin repository.

racheljpg · 2024-03-05T14:39:18Z

/retest

elmiko

changes are looking good, we need some cleanup and more tests

elmiko · 2024-03-05T20:30:37Z

pkg/controller/machineautoscaler/machinetarget_test.go

+				annotationsutil.CpuKeyDeprecated: "1",
+			},
+		},
+	}


i think we need more test cases here to exercise all the code paths in that function

Hey Mike, thank you for the feedback. I've added some more test cases to test the other annotations in the SetAnnotations function

Also added a second commit to make some changes to commits/names for clarity!

these tests are looking great @racheljpg !

racheljpg · 2024-03-06T14:43:59Z

pkg/controller/machineautoscaler/machinetarget_test.go

+}
+
+// test checkscalefromzeroannotations on a new machine
+func TestScaleFromZeroAnnotations(t *testing.T) {


@elmiko Do you think it's worth adding this as a test case above to make things neater, or is it fine to leave it out here on it's own?

this seems like we are testing the same functionality as above, i would add it as a case there.

It actually seems like we are doing what I was trying to test there above on line 389, so I've removed that function entirely :)

elmiko

apologies for the delay on getting back here, this is looking good to me, just a couple minor nits and we can probably merge this.

elmiko · 2024-03-14T19:01:02Z

pkg/controller/machineautoscaler/machinetarget_test.go

+			expectedNewAnnotation: map[string]string{
+				annotationsutil.GpuCountKey:           "1",
+				annotationsutil.GpuCountKeyDeprecated: "1",
+				annotationsutil.GpuTypeKey:            "1",


just to make this a little more realistic, i would change this value to nvidia.com/gpu, that is how it will look in a live cluster.

elmiko · 2024-03-14T19:01:25Z

pkg/controller/machineautoscaler/machinetarget_test.go

+				annotationsutil.CpuKeyDeprecated: "1",
+			},
+		},
+	}


these tests are looking great @racheljpg !

elmiko · 2024-03-14T19:04:32Z

pkg/controller/machineautoscaler/machinetarget_test.go

+}
+
+// test checkscalefromzeroannotations on a new machine
+func TestScaleFromZeroAnnotations(t *testing.T) {


this seems like we are testing the same functionality as above, i would add it as a case there.

racheljpg · 2024-03-15T15:42:44Z

Hello @elmiko, no worries, and thank you. I've changed the value of the GpuTypeKey and removed a redundant test. Thanks!

racheljpg · 2024-03-28T13:34:13Z

Hello!
I've tested this manually (thanks @elmiko) and can confirm that the new annotations show up on a MachineSet. :)

apiVersion: machine.openshift.io/v1beta1
kind: MachineSet
metadata:
  annotations:
    machine.openshift.io/GPU: '0'
    autoscaling.openshift.io/machineautoscaler: openshift-machine-api/worker-us-east-1a
    machine.openshift.io/memoryMb: '16384'
    capacity.cluster-autoscaler.kubernetes.io/gpu-type: nvidia.com/gpu
    capacity.cluster-autoscaler.kubernetes.io/labels: kubernetes.io/arch=amd64
    machine.openshift.io/cluster-api-autoscaler-node-group-min-size: '1'
    capacity.cluster-autoscaler.kubernetes.io/memory: '17179869184'
    capacity.cluster-autoscaler.kubernetes.io/cpu: '4'
    capacity.cluster-autoscaler.kubernetes.io/gpu-count: '0'
    machine.openshift.io/vCPU: '4'
    machine.openshift.io/cluster-api-autoscaler-node-group-max-size: '12'

elmiko

/approve
/lgtm

openshift-ci · 2024-03-28T13:42:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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~~ [elmiko]

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

openshift-ci · 2024-03-28T15:56:10Z

@racheljpg: all tests passed!

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-bot · 2024-03-28T19:28:57Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-autoscaler-operator-container-v4.16.0-202403281444.p0.g287595b.assembly.stream.el9 for distgit ose-cluster-autoscaler-operator.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 4, 2024

openshift-ci bot requested review from JoelSpeed and RadekManak March 4, 2024 17:00

racheljpg force-pushed the upstreamannotations branch 2 times, most recently from 3285a25 to 1b43a09 Compare March 4, 2024 17:41

racheljpg force-pushed the upstreamannotations branch from 1b43a09 to 7b7d287 Compare March 5, 2024 15:13

elmiko reviewed Mar 5, 2024

View reviewed changes

Update CAO to add upstream annotations

368e157

racheljpg force-pushed the upstreamannotations branch from 7b7d287 to 368e157 Compare March 6, 2024 14:42

racheljpg commented Mar 6, 2024

View reviewed changes

elmiko reviewed Mar 14, 2024

View reviewed changes

racheljpg force-pushed the upstreamannotations branch from 2de239e to 77cc8a9 Compare March 15, 2024 15:41

Update comments and function names for clarity

b7cce73

racheljpg force-pushed the upstreamannotations branch from 77cc8a9 to b7cce73 Compare March 21, 2024 09:36

elmiko reviewed Mar 28, 2024

View reviewed changes

openshift-ci bot assigned elmiko Mar 28, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2024

openshift-merge-bot bot merged commit 287595b into openshift:master Mar 28, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPCLOUD-2493: Update CAO to add upstream annotations #316

OCPCLOUD-2493: Update CAO to add upstream annotations #316

racheljpg commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024 •

edited by openshift-ci bot

racheljpg commented Mar 5, 2024

elmiko left a comment

elmiko Mar 5, 2024

racheljpg Mar 6, 2024

racheljpg Mar 6, 2024

elmiko Mar 14, 2024

racheljpg Mar 6, 2024

elmiko Mar 14, 2024

racheljpg Mar 15, 2024

elmiko left a comment

elmiko Mar 14, 2024

elmiko Mar 14, 2024

elmiko Mar 14, 2024

racheljpg commented Mar 15, 2024

racheljpg commented Mar 28, 2024 •

edited

elmiko left a comment

openshift-ci bot commented Mar 28, 2024

openshift-ci bot commented Mar 28, 2024

openshift-bot commented Mar 28, 2024

OCPCLOUD-2493: Update CAO to add upstream annotations #316

OCPCLOUD-2493: Update CAO to add upstream annotations #316

Conversation

racheljpg commented Mar 4, 2024

openshift-ci-robot commented Mar 4, 2024 • edited by openshift-ci bot

racheljpg commented Mar 5, 2024

elmiko 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

elmiko 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

racheljpg commented Mar 15, 2024

racheljpg commented Mar 28, 2024 • edited

elmiko left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 28, 2024

openshift-ci bot commented Mar 28, 2024

openshift-bot commented Mar 28, 2024

openshift-ci-robot commented Mar 4, 2024 •

edited by openshift-ci bot

racheljpg commented Mar 28, 2024 •

edited