Federated deployment controller - part 1 #34109
Conversation
mwielgus
added
retest-not-required
release-note-none
k8s-github-robot
added
the
size/XL
|
Jenkins unit/integration failed for commit e826dc7a7e2e3c8d75d98657c955f2fbf863ae44. Full PR test history. The magic incantation to run this job again is |
|
Thanks @mwielgus. Let me know when it's ready for review. |
ghost
added
the
priority/important-soon
|
PS: We will need a release note for this one, so I've switched the label. |
ghost
added
release-note-label-needed
and removed
release-note-none
mwielgus
force-pushed
the
fed-deployment-controller
branch
from
e826dc7 to
48307b0
Compare
|
The PR is ready for the review. The mentioned items will be don in follow-up prs. I'm not a big fan of sending >1k LOC PRs :). |
mwielgus
added
release-note
|
Jenkins GCI GKE smoke e2e failed for commit 48307b0. Full PR test history. The magic incantation to run this job again is |
| rs := newDeploymentWithReplicas("rs", 9) | ||
| rs, _ = fedclientset.Extensions().Deployments(apiv1.NamespaceDefault).Create(rs) | ||
| fedrswatch.Add(rs) | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Rather poll until success below. 1 second is arbitrary, slows down the test, and might lead to flakes in a highly contended environment.
There was a problem hiding this comment.
Fixed. The same needs to be applied to the original replicaset controller tests.
| } | ||
| } | ||
|
|
||
| func TestDeploymentController(t *testing.T) { |
There was a problem hiding this comment.
Please add log messages, or at least comments to explain what you're testing at each step, and why.
| fedUpdater fedutil.FederatedUpdater | ||
|
|
||
| deploymentBackoff *flowcontrol.Backoff | ||
| // For events |
| fdc.reconcileDeploymentsOnClusterChange() | ||
| }) | ||
|
|
||
| for !fdc.isSynced() { |
There was a problem hiding this comment.
This could end up being a relatively tight endless loop. You'll need logging and exponential backoff.
There was a problem hiding this comment.
Added logging. Exponential backoff is not needed here. If the controller cannot sync then it is useless.
|
|
||
| go func() { | ||
| select { | ||
| case <-time.After(time.Minute): |
There was a problem hiding this comment.
There are a lot of seemingly arbitrary delay times in this code. Please lift them into variables or constants at the top of the file, and explain in comments what each time period is for, and how it's value was arrived at.
There was a problem hiding this comment.
Added comment. No need to make it a constant.
| } | ||
| } | ||
|
|
||
| func (fdc *DeploymentController) schedule(fd *extensionsv1.Deployment, clusters []*fedv1.Cluster, |
There was a problem hiding this comment.
Please add a method comment describing the arguments.
| current map[string]int64, estimatedCapacity map[string]int64) map[string]int64 { | ||
| // TODO: integrate real scheduler | ||
|
|
||
| planer := fdc.defaultPlanner |
| // TODO: integrate real scheduler | ||
|
|
||
| planer := fdc.defaultPlanner | ||
| fdPref, err := parseFederationDeploymentReference(fd) |
| glog.Info("Invalid Deployment specific preference, use default. deployment: %v, err: %v", fd.Name, err) | ||
| } | ||
| if fdPref != nil { // create a new planner if user specified a preference | ||
| planer = planner.NewPlanner(fdPref) |
| } | ||
| scheduleResult, overflow := planer.Plan(replicas, clusterNames, current, estimatedCapacity, | ||
| fd.Namespace+"/"+fd.Name) | ||
| // make sure the return contains clusters need to zero the replicas |
mwielgus
force-pushed
the
fed-deployment-controller
branch
from
48307b0 to
c9e771a
Compare
|
LGTM as an initial revision. I will give the followup PR's a more thorough review, as quite a bit of this code is likely to change soon in those. |
ghost
added
the
lgtm
|
Automatic merge from submit-queue |
Based on federated replicaset controller (copy + find/replace).
Remaining stuff:
cc: @quinton-hoole @kubernetes/sig-cluster-federation
Release note:
This change is