add KEP 238, to add controller revision

[Edwinhr716]

What type of PR is this?

/kind documentation

What this PR does / why we need it

Kep for the fix for #238 #240 #281

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?

[Edwinhr716]

@kerthcet PTAL as well

[ahg-g]

what if there was another update that caused this difference? or is that not possible (meaning do we block updates if an update is currently in progress)?

[Edwinhr716]

We don't block updates if an update is in progress.

It is a good point, there's a scenario where another update causes the difference. If an update is started from 1 to 2, and then is updated to 3, if the update to 3 happens when the leader pod has been updated to 2, but the worker pod hasn't, then the worker pod will be created with 1, causing the discrepancy again.

To address this, we could do something like this

func constructWorkerStatefulSetApplyConfiguration(currentRevision) {
   if updatedTemplateHash != leaderPod.Labels[templateHash] {
        originalLws, err := ApplyRevision(&lws, currentRevision)
        podTemplateSpec = *originalLws.WorkerTemplate.DeepCopy()
        if !TemplateHashMatches(leaderPod.Labels[templateHash], originalLws) {
              originalLws, err := GetRevision(leaderPod.Labels[templateHash])
         }
     }
}

// iterates through the list of revisions and returns the lws object with the matching template hash
func GetRevision(string templateHash) {
      revisions := ListRevisions()
      for _, revision := range revisions {
             lws, err := ApplyRevision(&lws, revision)
             if lws.Labels[templateHash] == templateHash {
                   return lws
            }
      }
}

Wdyt?

[ahg-g]

sounds good, so we try to find the correct revision by finding the one with a matching hash, makes sense.

[ahg-g]

can you clarify when updateRevision gets created/set?

[Edwinhr716]

updateRevision gets created when there isn't an existing revision that has already been created. So the pseudocode is

 func GetLeaderWorkerSetRevisions(ctx, r.Client, lws) {
    revisions := ListControllerRevisions()
    updateRevision := NewRevision(lws)
    equalRevisions := FindEqualRevisions(revisions, updateRevision)
    if len(equalRevisions) == 0 {
        CreateControllerRevision(lws, updateRevision)
    }else if equalRevision(revisions[len(revisions) - 1], equalRevisions[equalCount-1]) {
        // in this case, updateRevision is the same as currentRevision, so no update is occuring
        updateRevision = equalRevisions[equalCount-1]
   }
}

CurrentRevision doesn't get updated/changed unless lws.CurrentRevision is nil. So during an update, updateRevision and currentRevision will be different until the update is complete.

[Edwinhr716]

/retest

[ahg-g]

can you define what is currentReplicas, replicas and updatedReplicas?

[ahg-g]

I think we can simplify this as follows:

1. Create a ControllerRevision each time we set the leader sts with a different template hash (here);
2. In the pod reconciler, we simply lookup the revision with a matching template hash
3. Truncate history each time an update is done, leaving only the controllerRevision with a matching hash of the current template

With this, no need for maintain anything in status.

[Edwinhr716]

Makes sense, I'll update the KEP with those changes, and add changes to fix #281

[ahg-g]

Discussed offline:

• Nothing will be tracked in lws status
• We will use the template hash as a label on each new controller revision that gets created. This key is stored as a label on the leaders sts.

On each lws reconcile:

• Use the hash key on the leader sts to lookup the latest controller revision
• If one exists, compare the existing controller revision with the current lws spec:
  • If different, trigger an update
  • If the same, don't trigger an update
• If no one exists, create one and don't trigger an update
• Each time an update is done, trim the controller revisions and only keep the one with the same hash key as the current leaders sts

One each leader pod reconcile:

• Use the hash key to lookup the controller revision
• Use the worker template in the controller revision to create workers sts

[ahg-g]

we also need to check some parameters of the LWS spec itself, specifically the subdomain policy

[Edwinhr716]

Yes, equalLeaderWorkerTemplates would check both lws.spec.leaderWorkerTemplate and checking subdomainPolicy, also treating networkConfig == nil as subdomainShared just like it is done for templateHash

[ahg-g]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Edwinhr716

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 [ahg-g]

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