Refactor adminUpdate steps #3376

mociarain · 2024-02-02T14:36:20Z

Which issue this PR addresses:

A refactor to lay the groundwork for: https://issues.redhat.com/browse/ARO-5327

What this PR does / why we need it:

I've smashed all the various if blocks to give us another view on what this is doing and hopefully, this is clearer. I've preserved the step execution order (mostly) because it's not clear to me what exactly what dependencies are involved. Functionally, I believe this is identical to what we had before but now there is some extra repeats of certain idempotent steps.

Note: I noticed that in the everything path weren't actually running the m.renewMDSDCertificate step. This PR fixes this.

Some questions:

~~Is it valid to run this with all flags set to false? Technically, it would have done bootstrap 0 & 1 and step 0 but here I set it to panic.~~
It is valid according to the unit tests.
~~Is the hive stuff dependant on initializeOperatorDeployer?~~
No.

Related PUCM scipt PR: https://msazure.visualstudio.com/AzureRedHatOpenShift/_git/ARO-Scripts/pullrequest/9515547

Test plan for issue:

I've tested this and it's using my local RP to prove that:

The everything step runs everything happily.
The individual "branches" can be ran successfully.

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

https://msazure.visualstudio.com/AzureRedHatOpenShift/_git/WIKI-Designs/pullrequest/9515543

Future Ideas/Thoughts:

Don't grow GeneralFixesSteps
This is now a kind of dumping ground of random things that we should stop adding to IMO. It's only going to snowball and hopefully what I have done here shows how we could define extra groups and add them with correct context.
Break up GeneralFixesSteps
This would involve understanding the dependencies between steps and more understanding than me to group them better conceptually.

mociarain · 2024-02-02T14:37:08Z

pkg/cluster/install.go

+		// Block 8
+		steps.Action(m.renewMDSDCertificate),


This step has been added to the all/everything steps.

(I think it was a bug that it wasn't in everything)

mociarain · 2024-02-05T15:54:02Z

pkg/cluster/install.go

 )

+func concatMultipleSlices[T any](slices ...[]T) []T {


Is there a better place for super generic helpers like this?

Maybe somewhere like https://github.com/Azure/ARO-RP/tree/master/pkg/util/stringutils but with a rename to utils, but that nested utils folder makes me sad, so dunno

something something rename to 'common' something something

Looks like a good use of generics here.

Golang 1.22 includes the new slices.Concat method which would replace this function. I recommend we include the implementation ourselves for now. Nothing stands out immediately as something that can't be done in Go 1.20.

As far as function placement is concerned, I'd recommend pkg/util maybe under a folder like generics or helpers although that can end up being a bucket for everything.

mociarain · 2024-02-07T14:40:04Z

pkg/cluster/adminupdate_test.go

+		"[Action startVMs-fm]",
+		"[Condition apiServersReady-fm, timeout 30m0s]",


These now happen earlier in the ordering.

nwnt · 2024-02-07T16:12:33Z

pkg/cluster/install.go

 )

+func concatMultipleSlices[T any](slices ...[]T) []T {
+	result := []T{}


Nit: if the final size of the result array is known, we could use make to preallocate the capacity. This will avoid unnecessary new slice memory allocations.

So I considered something like this but I think I would need to loop over the slices twice. Once, to get the length, then build the array, then loop over the slices again to populate the array. My heuristic is that more looping is bad and dynamically adding to a slice would be better.

Did you have a different implementation in mind?

Is my heuristic just plain wrong?

pkg/cluster/install.go

tiguelu · 2024-03-08T13:15:47Z

pkg/cluster/install.go

-			steps.Action(m.fixSSH),
-			// steps.Action(m.removePrivateDNSZone), // TODO(mj): re-enable once we communicate this out
-		)
+	// Generic fix-up actions that are fairly safe to always take, and don't require a running cluster


Curious about why the fixInfraID action is outside the list above. The actions in that list also comply with the description of the comment for this action (in the old code, the comment was actually for all of them).

My thinking was that before there were several generic tasks that happened for every path. These were all clearly bootstrapping tasks apart from this one which is actually changing the state of the cluster.

tiguelu · 2024-03-08T13:37:07Z

pkg/cluster/install.go

+		steps.Action(m.fixSREKubeconfig),
+		steps.Action(m.fixUserAdminKubeconfig),
+		steps.Action(m.createOrUpdateRouterIPFromCluster),


The actionpopulateDatabaseIntIP that populates the internal LB IP for the API into the DB was run before these 3 actions in the old code. I wonder if there was any requirement for that, since these 3 steps will require the API to be accessible.

Same wonder with the apiServersReady condition after starting the VMs which is now in getZerothSteps function before the DB IP fix runs, but it used to be after.

The reason I moved this down is because populateDatabaseIntIP patches doc.OpenShiftCluster.Properties.APIServerProfile.IntIP. This property in only 2 other tasks:

fixMCSCert

fixMCSUserData

For the startVMs task I moved that back into bootstrapping because it seems to fit there better conceptually and I'm assuming the tasks that come before it in the if blocks can't be pre-requisites because we start the VMs in each path but only run some tasks before we start the VMs in isEverything.

jaitaiwan

Mostly LGTM, just one point on the slice concat. Noting that I think it should be a public function.

jaitaiwan · 2024-03-10T23:45:21Z

pkg/cluster/install.go

 )

+func concatMultipleSlices[T any](slices ...[]T) []T {


Golang 1.22 includes the new slices.Concat method which would replace this function. I recommend we include the implementation ourselves for now. Nothing stands out immediately as something that can't be done in Go 1.20.

As far as function placement is concerned, I'd recommend pkg/util maybe under a folder like generics or helpers although that can end up being a bucket for everything.

mociarain · 2024-03-11T10:20:08Z

Closing in favour of: #3449

mociarain commented Feb 2, 2024

View reviewed changes

mociarain changed the title ~~[WIP] Lay it out clearly~~ Refactor adminUpdate steps Feb 5, 2024

mociarain commented Feb 5, 2024

View reviewed changes

mociarain marked this pull request as ready for review February 5, 2024 15:54

mociarain requested review from jewzaam, bennerv, hawkowl, rogbas, jharrington22, cblecker, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, Shivkumar13, yjst2012, jaitaiwan, anshulvermapatel, hlipsig and dofinn as code owners February 5, 2024 15:54

mociarain mentioned this pull request Feb 7, 2024

Make NSG Monitor run every 10 minutes #3343

Merged

mociarain force-pushed the tidy-up-adminUpdate branch 2 times, most recently from 3ad3651 to d595c41 Compare February 7, 2024 14:35

mociarain commented Feb 7, 2024

View reviewed changes

nwnt reviewed Feb 7, 2024

View reviewed changes

mociarain added 5 commits February 8, 2024 12:25

Lay it out clearly

5a84bf9

Break the steps up into groups

5527980

Factor out the common 0th setup/fixes

cb3f9e4

Update unit test

226d559

s/cutoff/cutOff

120d5a2

mociarain added 5 commits February 8, 2024 12:25

Factor out common bootstrap steps

aa45dc2

Update the tests

ebe5dff

Remove initializeOperatorDeployer for Hive steps

b8a5ab7

Tidy up

0a73156

Revise the updateOperator Task

68b23ca

mociarain force-pushed the tidy-up-adminUpdate branch from d595c41 to 68b23ca Compare February 8, 2024 11:25

mociarain added ready-for-review go Pull requests that update Go code labels Feb 8, 2024

Decouple OperatorUpdate and CertificateRenewal

6075829

mociarain requested a review from nwnt February 21, 2024 09:12

tiguelu reviewed Mar 8, 2024

View reviewed changes

jaitaiwan requested changes Mar 10, 2024

View reviewed changes

Move ConcatMultipleSlices in generic helpers

0bfce43

mociarain requested a review from jaitaiwan March 11, 2024 09:39

mociarain mentioned this pull request Mar 11, 2024

Fix adminUpdate steps #3449

Merged

2 tasks

mociarain closed this Mar 11, 2024

mociarain deleted the tidy-up-adminUpdate branch March 11, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor adminUpdate steps #3376

Refactor adminUpdate steps #3376

mociarain commented Feb 2, 2024 •

edited by tiguelu

Loading

mociarain Feb 2, 2024

mociarain Feb 5, 2024

mociarain Feb 5, 2024

dkeohane Feb 7, 2024

dkeohane Feb 7, 2024

nwnt Feb 7, 2024

jaitaiwan Mar 10, 2024

mociarain Feb 7, 2024

nwnt Feb 7, 2024

mociarain Feb 8, 2024

tiguelu Mar 8, 2024

mociarain Mar 8, 2024

tiguelu Mar 8, 2024 •

edited

Loading

mociarain Mar 8, 2024

jaitaiwan left a comment

jaitaiwan Mar 10, 2024

mociarain commented Mar 11, 2024

		"[Action startVMs-fm]",
		"[Condition apiServersReady-fm, timeout 30m0s]",

Refactor adminUpdate steps #3376

Refactor adminUpdate steps #3376

Conversation

mociarain commented Feb 2, 2024 • edited by tiguelu 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?

Future Ideas/Thoughts:

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

tiguelu Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaitaiwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mociarain commented Mar 11, 2024

mociarain commented Feb 2, 2024 •

edited by tiguelu

Loading

tiguelu Mar 8, 2024 •

edited

Loading