Add worker profile status #3053

gouthamMN · 2023-07-21T18:29:27Z

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-2124

What this PR does / why we need it:

OpenShiftCluster GET API returns enriched worker profile data under a new field "workerProfilesStatus". This change is introduced only to new API. More details can be found on Design Doc

Any API call made to endpoints /openShiftClusters or /openShiftCluster with GET method using newer API versions (>= v2023-09-04) will return the enriched worker profile data under a new field "workerProfilesStatus" in the response and "workerProfiles" filed will contain the last input that was passed in PUT/PATCH request body for cluster creation or update.

The response and behavior of any older APIs versions (< v2023-09-04) remain unchanged i.e, only "workerProfiles" filed exists in the response.

Test plan for issue:

Existing unit tests and E2E test copied over from previous API.
As suggested in the comments, performed the Regression Tests:

Created a ARO clustered from az cli **az aro create**.
Performed ARO cluster update **az aro update**.

From the test, it was confirmed that there were no changes in the behavior of the existing stable API (v2023-04-01). The new API (v2023-09-04) cannot be tested at this point as the az cli is not updated to use it.

Is there any documentation that needs to be updated for this PR?

N/A

weinong

LGTM

lranjbar · 2023-07-25T15:11:05Z

The code LGTM. Let's identify the code using these API endpoints and check if they need updates in a subsequent PR.

bennerv

I think we're missing updating any usage to point to the new workerProfilesStatus.

Anywhere we use kubeSubnets or the workerProfiles field internally may be subject to breakage or not complete / expected functionality. We should explore them, as the PRs should be merged together to prevent any breaking changes by introducing this change.

bennerv · 2023-07-26T22:42:55Z

pkg/swagger/typewalker.go

@@ -141,6 +141,9 @@ func (tw *typeWalker) schemaFromType(t types.Type, deps map[*types.Named]struct{

 				properties := tw.schemaFromType(field.Type(), deps)
 				properties.Description = strings.Trim(node.Doc.Text(), "\n")
+				if field.Name() == "WorkerProfilesStatus" {
+					properties.ReadOnly = true
+				}


does ARM prevent the user passing in values for this if we set the swagger specification to ReadOnly? If not, we should do a server-side check to ensure the user can't actually patch this.

Updated openshiftcluster_validatestatic.go to validate workerProfilesStatus filed is nil in the input request. This gets called during preflightvalidation.

bennerv · 2023-07-26T22:49:17Z

pkg/api/v20200430/openshiftcluster_convert.go

+		workerProfiles := oc.Properties.WorkerProfiles
+
+		// Use enriched worker profile data when available
+		if oc.Properties.WorkerProfilesStatus != nil {
+			workerProfiles = oc.Properties.WorkerProfilesStatus
+		}
+
+		out.Properties.WorkerProfiles = make([]WorkerProfile, 0, len(workerProfiles))
+		for _, p := range workerProfiles {


Wouldn't this generate new examples for the older API?

Nope, because under openshiftcluster_example.go package we set the WorkerProfilesStatus field to nil before calling the converter function (ToExternal). This will ensure that the swagger examples remain unchanged.

Older APIs swagger examples never showed the enriched worker profile data under workerProfiles field. So I decided to keep it same.

bennerv · 2023-07-26T22:50:07Z

pkg/api/v20230904/openshiftcluster.go

@@ -64,6 +64,9 @@ type OpenShiftClusterProperties struct {
 	// The cluster worker profiles.
 	WorkerProfiles []WorkerProfile `json:"workerProfiles,omitempty"`

+	// The cluster worker profiles status.
+	WorkerProfilesStatus []WorkerProfile `json:"workerProfilesStatus,omitempty"`


i'm wondering if we should make a new type for this. In the update code, we never actually set some of the parameters that exist within a workerProfile struct.

WorkerProfilesStatus - I see all the field are set/referenced under worker_profile.go package while performing the enrichment.

pkg/frontend/openshiftcluster_putorpatch.go

pkg/util/clusterdata/worker_profile.go

gouthamMN · 2023-07-28T14:49:07Z

I think we're missing updating any usage to point to the new workerProfilesStatus.

Anywhere we use kubeSubnets or the workerProfiles field internally may be subject to breakage or not complete / expected functionality. We should explore them, as the PRs should be merged together to prevent any breaking changes by introducing this change.

These are the list of ARO-RP pkgs that had reference to workerProfiles filed and I had updated whichever needs to be updated to workerProfilesStatus. Let me know if you think I'm missing anything.

github-actions · 2023-07-28T17:13:12Z

Please rebase pull request.

s-amann

I believe we need to at least rethink the conversion logic so it can account for nil workerProfileStatus before workerProfiles since the enrichment in this PR will never populate workerProfiles. See comments below.

s-amann · 2023-08-08T20:46:17Z

pkg/util/clusterdata/worker_profile.go

@@ -102,10 +102,10 @@ func (ce machineClientEnricher) Enrich(
 	oc.Lock.Lock()
 	defer oc.Lock.Unlock()

-	oc.Properties.WorkerProfiles = workerProfiles


after further thinking, I believe this will break our admin api. Since the doc will not have workerProfiles set, when the conversion is made, workerProfiles is nil and so the output will not have workerProfiles since we first check for workerProfiles and then check for workerProfileStatus. Any automation in place which relies on the workerProfiles via admin API would need to be changed.

I think an alternative approach, would be to leave the doc the same (always have it be enriched via workerProfiles) and for new API versions we can do the conversion.

Updated workerProfiles references as suggested by Ben.

bennerv · 2023-08-08T22:45:50Z

I haven't had the chance to review the list of places it's been used yet, but we need to be careful this change doesn't break existing functionality.

Think this:

Essentially this change makes the workerProfile immutable. the customer sets it on creation, but moving forward, they actually can't update the field and the RP does anything about it. Probably not the best behavior, but the reality
The workerProfilesStatus field represents the in-cluster state of the world. We use this in multiple places: to gather all subnets and ensure we have the proper RBAC on the vnets.
When a customer performs day2 operations, they can technically bring worker nodes from a completely different subnet
Because of 3, we need to update our usage everywhere workerProfiles is used. We always want to reconcile or use the state of the in-cluster machines/machinesets, otherwise this change may have undesired consequences.

I would say generally we can proceed by:

Anywhere workerProfiles is used, change it to workerProfilesStatus if workerProfilesStatus != nil || len(workerProfilesStatus) > 0. Otherwise use workerProfiles.
When we update the azure-cli to use the latest API version, we need to take the intersection of workerProfiles and workerProfilesStatus to ensure we have the proper FPSP and CSP RBAC to work.

gouthamMN · 2023-08-10T22:03:43Z

I haven't had the chance to review the list of places it's been used yet, but we need to be careful this change doesn't break existing functionality.

Think this:

Essentially this change makes the workerProfile immutable. the customer sets it on creation, but moving forward, they actually can't update the field and the RP does anything about it. Probably not the best behavior, but the reality

The workerProfilesStatus field represents the in-cluster state of the world. We use this in multiple places: to gather all subnets and ensure we have the proper RBAC on the vnets.

When a customer performs day2 operations, they can technically bring worker nodes from a completely different subnet

Because of 3, we need to update our usage everywhere workerProfiles is used. We always want to reconcile or use the state of the in-cluster machines/machinesets, otherwise this change may have undesired consequences.

I would say generally we can proceed by:

Anywhere workerProfiles is used, change it to workerProfilesStatus if workerProfilesStatus != nil || len(workerProfilesStatus) > 0. Otherwise use workerProfiles.

When we update the azure-cli to use the latest API version, we need to take the intersection of workerProfiles and workerProfilesStatus to ensure we have the proper FPSP and CSP RBAC to work.

made changes to the workerProfiles references as suggested.

pkg/api/util/subnet/subnet.go

s-amann · 2023-08-21T12:10:27Z

/azp run e2e

azure-pipelines · 2023-08-21T12:10:38Z

Azure Pipelines successfully started running 1 pipeline(s).

petrkotas

Hi @gouthamMN thank you for the PR. I believe it should work as is, but this has potential to go in regression. Meaning, I approve, but we need at least one more approve before merging. @bennerv ?

gouthamMN · 2023-08-22T15:42:33Z

@bennerv - I have added JIRA item https://issues.redhat.com/browse/ARO-4016 to work on updating the azure-cli to take the intersection of workerProfiles and workerProfilesStatus to ensure we have the proper FPSP and CSP RBAC to work. Once this PR is merged I will update the azure-cli.

s-amann

lgtm, considering the risk of regression and the importance of workerprofiles, can we deploy this branch to INT, create a cluster and run an az aro update on it, PUCM it, and see what the output of the GetCluster GA is?

gouthamMN · 2023-08-28T20:26:48Z

lgtm, considering the risk of regression and the importance of workerprofiles, can we deploy this branch to INT, create a cluster and run an az aro update on it, PUCM it, and see what the output of the GetCluster GA is?

As suggested, performed the Regression Tests:

Created a ARO clustered from az cli **az aro create**.
Performed ARO cluster update **az aro update**.

From the test it was confirmed there were no changes in the behavior of the existing stable API (v2023-04-01). The new API (v2023-09-04) cannot be tested at this point as the az cli is not updated to use it.

stale, appears concerns have been addressed

* add workerProfilesStatus field to hold the enriched worker profile data * update swagger * swagger examples * update clients * resolve golint * update defaults * validate worker Profile status is nil in input request * make client changes after rebase * rebase * update workerProfiles references and UTs * fix golint errors * remove duplicate logic of verifing workerProfilesStatus not nil --------- Co-authored-by: gniranjan <[email protected]>

gouthamMN requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, facchettos, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, ellis-johnson, yjst2012 and anshulvermapatel as code owners July 21, 2023 18:29

gouthamMN requested a review from weinong July 21, 2023 18:33

gouthamMN added ready-for-review size-medium Size medium labels Jul 21, 2023

weinong previously approved these changes Jul 21, 2023

View reviewed changes

gouthamMN added the LGTM Looks Good To Me label Jul 25, 2023

gouthamMN mentioned this pull request Jul 26, 2023

Fixing ARO API to enable Terraform Provider for ARO #2983

Closed

bennerv previously requested changes Jul 26, 2023

View reviewed changes

gouthamMN dismissed weinong’s stale review via 6018b46 July 27, 2023 18:27

github-actions bot added the needs-rebase branch needs a rebase label Jul 28, 2023

gouthamMN force-pushed the addWorkerProfileStatus branch from aef1db6 to 560399e Compare July 28, 2023 18:29

github-actions bot removed the needs-rebase branch needs a rebase label Jul 28, 2023

gouthamMN removed the LGTM Looks Good To Me label Jul 28, 2023

gouthamMN added 2 commits August 2, 2023 13:12

Merge branch 'Azure:master' into addWorkerProfileStatus

bcfa64a

Merge branch 'Azure:master' into addWorkerProfileStatus

bb770b6

s-amann approved these changes Aug 7, 2023

View reviewed changes

gouthamMN added the LGTM Looks Good To Me label Aug 7, 2023

s-amann suggested changes Aug 8, 2023

View reviewed changes

gouthamMN removed the LGTM Looks Good To Me label Aug 8, 2023

gouthamMN and others added 3 commits August 9, 2023 16:21

Merge branch 'Azure:master' into addWorkerProfileStatus

8a6adad

Merge branch 'Azure:master' into addWorkerProfileStatus

0d27f13

update workerProfiles references and UTs

d1fb933

fix golint errors

17d52c2

gouthamMN added the go Pull requests that update Go code label Aug 11, 2023

gouthamMN commented Aug 16, 2023

View reviewed changes

pkg/api/util/subnet/subnet.go Outdated Show resolved Hide resolved

remove duplicate logic of verifing workerProfilesStatus not nil

70ca268

gouthamMN removed the size-medium Size medium label Aug 17, 2023

Merge branch 'Azure:master' into addWorkerProfileStatus

999bd60

gouthamMN requested review from Shivkumar13 and hlipsig as code owners August 18, 2023 17:30

bennerv added the next-release To be included in the next RP release rollout label Aug 21, 2023

petrkotas approved these changes Aug 22, 2023

View reviewed changes

s-amann approved these changes Aug 25, 2023

View reviewed changes

s-amann requested a review from bennerv September 5, 2023 17:15

s-amann merged commit dba48c1 into Azure:master Sep 5, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add worker profile status #3053

Add worker profile status #3053

gouthamMN commented Jul 21, 2023 •

edited

Loading

weinong left a comment

lranjbar commented Jul 25, 2023

bennerv left a comment

bennerv Jul 26, 2023

gouthamMN Jul 27, 2023

bennerv Jul 26, 2023

gouthamMN Jul 27, 2023 •

edited

Loading

bennerv Jul 26, 2023

gouthamMN Jul 27, 2023

gouthamMN commented Jul 28, 2023

github-actions bot commented Jul 28, 2023

s-amann left a comment

s-amann Aug 8, 2023 •

edited

Loading

gouthamMN Aug 10, 2023 •

edited

Loading

bennerv commented Aug 8, 2023

gouthamMN commented Aug 10, 2023 •

edited

Loading

s-amann commented Aug 21, 2023

azure-pipelines bot commented Aug 21, 2023

petrkotas left a comment

gouthamMN commented Aug 22, 2023 •

edited

Loading

s-amann left a comment

gouthamMN commented Aug 28, 2023 •

edited

Loading

Add worker profile status #3053

Add worker profile status #3053

Conversation

gouthamMN commented Jul 21, 2023 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

weinong left a comment

Choose a reason for hiding this comment

lranjbar commented Jul 25, 2023

bennerv left a comment

Choose a reason for hiding this comment

bennerv Jul 26, 2023

Choose a reason for hiding this comment

gouthamMN Jul 27, 2023

Choose a reason for hiding this comment

bennerv Jul 26, 2023

Choose a reason for hiding this comment

gouthamMN Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

bennerv Jul 26, 2023

Choose a reason for hiding this comment

gouthamMN Jul 27, 2023

Choose a reason for hiding this comment

gouthamMN commented Jul 28, 2023

github-actions bot commented Jul 28, 2023

s-amann left a comment

Choose a reason for hiding this comment

s-amann Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

gouthamMN Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

bennerv commented Aug 8, 2023

gouthamMN commented Aug 10, 2023 • edited Loading

s-amann commented Aug 21, 2023

azure-pipelines bot commented Aug 21, 2023

petrkotas left a comment

Choose a reason for hiding this comment

gouthamMN commented Aug 22, 2023 • edited Loading

s-amann left a comment

Choose a reason for hiding this comment

gouthamMN commented Aug 28, 2023 • edited Loading

gouthamMN commented Jul 21, 2023 •

edited

Loading

gouthamMN Jul 27, 2023 •

edited

Loading

s-amann Aug 8, 2023 •

edited

Loading

gouthamMN Aug 10, 2023 •

edited

Loading

gouthamMN commented Aug 10, 2023 •

edited

Loading

gouthamMN commented Aug 22, 2023 •

edited

Loading

gouthamMN commented Aug 28, 2023 •

edited

Loading