Consolidate claim assignment into clusterpool_controller #1474

2uasimojo · 2021-07-23T22:14:33Z

To avoid timing issues and properly count assigned ClusterDeployments and ClusterClaims, this commit moves the assignment of ClusterClaims (updating their reference to their assigned CD) from the clusterclaim_controller into the clusterpool_controller, right next to the assignment of ClusterDeployments (updating their reference to their assigned ClusterClaim).

It also adds code to double-check that assignments are in sync (claims assigned to the clusters that ref them, and vice versa) before proceeding with pool math and further assignments. This should help eliminate assignment conflicts, and "heal" partial updates (e.g. assigning the claim succeeds, but assigning the CD fails).

HIVE-1599

2uasimojo · 2021-07-23T22:15:19Z

First pass at the code side. TODO:

Clean up ordering of methods in collections.go
Fix UT
Add moar UT
Live test

codecov · 2021-07-27T21:18:37Z

Codecov Report

Merging #1474 (d8180cb) into master (3de9e02) will increase coverage by 1.02%.
The diff coverage is 69.59%.

@@            Coverage Diff             @@
##           master    #1474      +/-   ##
==========================================
+ Coverage   41.23%   42.26%   +1.02%     
==========================================
  Files         334      335       +1     
  Lines       30265    31012     +747     
==========================================
+ Hits        12481    13108     +627     
- Misses      16722    16800      +78     
- Partials     1062     1104      +42

Impacted Files	Coverage Δ
...g/controller/clusterpool/clusterpool_controller.go	`56.56% <58.82%> (-2.93%)`	⬇️
pkg/controller/clusterpool/collections.go	`70.76% <70.76%> (ø)`
...controller/clusterclaim/clusterclaim_controller.go	`60.65% <100.00%> (+0.06%)`	⬆️
pkg/controller/utils/errorscrub.go	`100.00% <0.00%> (ø)`
pkg/controller/utils/utils.go	`79.47% <0.00%> (+0.32%)`	⬆️
.../clusterdeployment/clusterdeployment_controller.go	`69.18% <0.00%> (+9.26%)`	⬆️

2uasimojo

Some notes for reviewers inline

pkg/controller/clusterpool/clusterpool_controller.go

pkg/controller/clusterpool/clusterpool_controller_test.go

2uasimojo · 2021-07-27T22:07:35Z

/assign @abhinavdahiya
/cc @dgoodwin @jnpacker

abhinavdahiya

did a first pass.

the collection is feeling a little weird, because side-effect functions leave the object not actionable again as they become stale
we are treating both claims and cds as source of truth when for safety we should only treat the cds as that. (claims are user editable)
View functions update the collection (sorting) when makes them incompatible with concurrent calls.

pkg/controller/clusterpool/clusterpool_controller.go

pkg/controller/clusterpool/collections.go

abhinavdahiya · 2021-07-30T16:17:03Z

pkg/controller/clusterpool/collections.go

+	// ByName returns the named ClusterDeployment from the cdLookup, or nil if no CD by that name exists.
+	ByName(string) *hivev1.ClusterDeployment
+	// Installing returns the list of ClusterDeployments in the process of being installed. These are
+	// not available for claim assignment.


These are not available for claim assignment.

doesn't seem like this function should say what is assignable?

That's totally its job. The implication is that you couldn't, for example, do:

for cd in cds.Installing() { cd.Assign() }

I don't think so, installing shouldn't say what you can do with the cd that are in this state.. that the user does with these is completely up to them. and in your example Assign() functions defines what type of cd can be assigned. Assign function should say that it should pass only assignable cd.

These structs/methods are 100% designed to conform to and enforce the architecture of the clusterpool controller, and that architecture dictates that we don't assign CDs that are Installing. If future-me is reading, say, assignClustersToClaims and says, "Whoah, we're only using Assignable() CDs -- was that a mistake??" I will go look at the comments for Installing and at least see that it was done on purpose.

I've added some words to the docstrings for cdCollection.Assign and assignClustersToClaims. But I'd like to keep these words here.

abhinavdahiya · 2021-07-30T16:18:30Z

pkg/controller/clusterpool/collections.go

+	// be assigned first to minimize claim response time.
+	// - Running CDs, oldest first
+	// - Resuming CDs, in order of least recently resumed (soonest likely to become Running)
+	// - Hibernating CDs, oldest first
+	// - Anything else, oldest first. (These should probably be skipped. TODO?)


doesn't seem like a good place for interface to define this specific behavior. the implementation should be the one in control, otherwise it's too strict.

The interfaces are gone. I'd like to keep this in the docstring of the impl itself.

pkg/controller/clusterpool/collections.go

abhinavdahiya · 2021-08-02T16:46:40Z

pkg/controller/clusterpool/collections.go

+
+// SyncCDAssignments makes sure each claim which purports to be assigned has the correct CD
+// assigned to it, updating the CD and/or claim on the server as necessary.
+func (claims *claimCollection) SyncCDAssignments(c client.Client, cds cdLookup, logger log.FieldLogger) {


why does this not return an error?

It's best-effort. It logs problems it encounters, but we don't want those problems to short-circuit the Reconcile. (I could have bubbled the errors up and ignored them in the caller, but didn't see the need.)

abhinavdahiya · 2021-08-02T16:51:34Z

pkg/controller/clusterpool/collections.go

+
+// SyncClaimAssignments makes sure each ClusterDeployment which purports to be assigned has the
+// correct claim assigned to it, updating the CD and/or claim on the server as necessary.
+func (cds *cdCollection) SyncClaimAssignments(c client.Client, claims claimLookup, logger log.FieldLogger) {


why does this not return an error?

It's best-effort. It logs problems it encounters, but we don't want those problems to short-circuit the Reconcile. (I could have bubbled the errors up and ignored them in the caller, but didn't see the need.)

pkg/controller/clusterpool/collections.go

pkg/controller/clusterpool/clusterpool_controller.go

pkg/controller/clusterpool/collections.go

2uasimojo · 2021-08-03T19:19:47Z

the collection is feeling a little weird, because side-effect functions leave the object not actionable again as they become stale

I don't think they become stale. I responded to one inline comment about this; were there other places this was of concern?

we are treating both claims and cds as source of truth when for safety we should only treat the cds as that. (claims are user editable)

I think this was addressed in an inline comment, yes?

View functions update the collection (sorting) when makes them incompatible with concurrent calls.

Moved sorting out of getters per this comment.

pkg/controller/clusterpool/collections.go

pkg/controller/clusterpool/clusterpool_controller.go

To avoid timing issues and properly count assigned ClusterDeployments and ClusterClaims, this commit moves the assignment of ClusterClaims (updating their reference to their assigned CD) from the clusterclaim_controller into the clusterpool_controller, right next to the assignment of ClusterDeployments (updating their reference to their assigned ClusterClaim). It also adds code to double-check that assignments are in sync (claims assigned to the clusters that ref them, and vice versa) before proceeding with pool math and further assignments. This should help eliminate assignment conflicts, and "heal" partial updates (e.g. assigning the claim succeeds, but assigning the CD fails). HIVE-1599

2uasimojo · 2021-08-05T22:57:09Z

Note: This is now stacked on #1489. Doesn't matter to me if we merge both commits here or separately.

abhinavdahiya · 2021-08-10T16:52:49Z

Note: This is now stacked on #1489. Doesn't matter to me if we merge both commits here or separately.

might want to rebase as 1489 merged?

abhinavdahiya

/lgtm

openshift-ci · 2021-08-10T17:07:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, abhinavdahiya

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [2uasimojo,abhinavdahiya]

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

openshift-bot · 2021-08-10T17:46:46Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 23, 2021

openshift-ci bot requested review from abhinavdahiya and akhil-rane July 23, 2021 22:14

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2021

2uasimojo force-pushed the consolidate-claiming branch 3 times, most recently from 9f27bcc to 8c0ecb5 Compare July 27, 2021 21:18

2uasimojo changed the title ~~WIP: Consolidate claim assignment into clusterpool_controller~~ Consolidate claim assignment into clusterpool_controller Jul 27, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2021

2uasimojo force-pushed the consolidate-claiming branch from 8c0ecb5 to f1f7e59 Compare July 27, 2021 21:57

2uasimojo commented Jul 27, 2021

View reviewed changes

openshift-ci bot assigned abhinavdahiya Jul 27, 2021

openshift-ci bot requested review from dgoodwin and jnpacker July 27, 2021 22:07

abhinavdahiya reviewed Aug 2, 2021

View reviewed changes

2uasimojo force-pushed the consolidate-claiming branch from f1f7e59 to 2852c47 Compare August 3, 2021 19:20

2uasimojo mentioned this pull request Aug 3, 2021

ClusterPool:ClusterDeployment version & condition #1484

Merged

dgoodwin reviewed Aug 4, 2021

View reviewed changes

abhinavdahiya reviewed Aug 4, 2021

View reviewed changes

pkg/controller/clusterpool/clusterpool_controller.go Outdated Show resolved Hide resolved

abhinavdahiya reviewed Aug 4, 2021

View reviewed changes

pkg/controller/clusterpool/clusterpool_controller.go Show resolved Hide resolved

abhinavdahiya reviewed Aug 4, 2021

View reviewed changes

pkg/controller/clusterpool/clusterpool_controller.go Show resolved Hide resolved

2uasimojo added 2 commits August 5, 2021 17:55

clusterpool_controller: Verify ClusterClaim Pending condition

f5d877b

2uasimojo force-pushed the consolidate-claiming branch from 2852c47 to d8180cb Compare August 5, 2021 22:56

abhinavdahiya reviewed Aug 10, 2021

View reviewed changes

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

openshift-ci bot merged commit ed4d2b3 into openshift:master Aug 10, 2021

2uasimojo deleted the consolidate-claiming branch August 11, 2021 14:22

2uasimojo mentioned this pull request Sep 20, 2021

ClusterPool RunningCount #1528

Merged

Consolidate claim assignment into clusterpool_controller #1474

Consolidate claim assignment into clusterpool_controller #1474

Uh oh!

Conversation

2uasimojo commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2uasimojo commented Jul 23, 2021

Uh oh!

codecov bot commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

2uasimojo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2uasimojo commented Jul 27, 2021

Uh oh!

abhinavdahiya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2uasimojo commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

2uasimojo commented Aug 5, 2021

Uh oh!

abhinavdahiya commented Aug 10, 2021

Uh oh!

abhinavdahiya left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 10, 2021

Uh oh!

openshift-bot commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

2uasimojo commented Jul 23, 2021 •

edited

Loading

codecov bot commented Jul 27, 2021 •

edited

Loading