KUBESAW-147: Space controller updates tier-hash label based on the NSTemplateTier.Status.Revisions field #1135
Conversation
…TemplateTier.Status.Revisions field Signed-off-by: Feny Mehta <[email protected]>
fbm3307
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
mfrancisc,
rsoaresd and
metlos
as code owners
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fbm3307 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:
Approvers can indicate their approval by writing |
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]> Signed-off-by: Feny Mehta <[email protected]>
Signed-off-by: Feny Mehta <[email protected]>
| Status: toolchainv1alpha1.NSTemplateTierStatus{ | ||
| Revisions: map[string]string{ | ||
| "base1ns-code-123456old": "base1ns-code-123456old", | ||
| "base1ns-dev-123456old": "base1ns-dev-123456old", | ||
| "base1ns-stage-123456old": "base1ns-stage-123456old", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
it looks like that this test whole test file doesn't actually verify the OutdatedTierSelector function, it looks more like a duplication of the tests that are in toolchain-common. I know that you didn't write the test, but could you please rewrite it? I can imagine that it could be a simple test calling the OutdatedTierSelector and using the selector to list Spaces (with multiple variations of the label), it doesn't have to be anything advanced.
| TemplateRef: tmplTier.Spec.ClusterResources.TemplateRef, | ||
| TemplateRef: tmplTier.Status.Revisions[tmplTier.Spec.ClusterResources.TemplateRef], | ||
| } | ||
| } | ||
| if len(tmplTier.Spec.Namespaces) > 0 { | ||
| s.Namespaces = make([]toolchainv1alpha1.NSTemplateSetNamespace, len(tmplTier.Spec.Namespaces)) | ||
| for i, ns := range tmplTier.Spec.Namespaces { | ||
| s.Namespaces[i] = toolchainv1alpha1.NSTemplateSetNamespace(ns) | ||
| s.Namespaces[i] = toolchainv1alpha1.NSTemplateSetNamespace{ | ||
| TemplateRef: tmplTier.Status.Revisions[ns.TemplateRef], | ||
| } |
There was a problem hiding this comment.
we should make sure that we start using the refs from the revisions map only when the map contains references for all TierTemplates from the Spec.
There can be still some small time gap when the NSTemplateTier.Spec is updated, but the status part doesn't contain the refs for all TierTemplates - in these cases, the refs in Namespaces would contain an empty string, which could trigger the deletion of the already existing namespace.
Let's add a simple "validate" function which validates that the revisions map contains refs for all TierTemplates, if not, then the controller should return an error
| base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates) | ||
| base1nsTier.Status.Revisions = tiertest.Base1nsRevision | ||
| // get an older base1ns tier (with outdated references) | ||
| olderbase1nsTier := tiertest.Base1nsTier(t, tiertest.PreviousBase1nsTemplates) | ||
| olderbase1nsTier.Status.Revisions = tiertest.OldTierRevision | ||
| otherTier := tiertest.OtherTier() | ||
| otherTier.Status.Revisions = tiertest.OtherTierRevision |
There was a problem hiding this comment.
why setting the revisions separately, and not as part of the functions that create the tiers?
| CurrentBase1nsTemplates.Namespaces[0].TemplateRef: CurrentBase1nsTemplates.Namespaces[0].TemplateRef, | ||
| CurrentBase1nsTemplates.Namespaces[1].TemplateRef: CurrentBase1nsTemplates.Namespaces[1].TemplateRef, | ||
| CurrentBase1nsTemplates.Namespaces[2].TemplateRef: CurrentBase1nsTemplates.Namespaces[2].TemplateRef, | ||
| CurrentBase1nsTemplates.ClusterResources.TemplateRef: CurrentBase1nsTemplates.ClusterResources.TemplateRef, | ||
| CurrentBase1nsTemplates.SpaceRoles["admin"].TemplateRef: CurrentBase1nsTemplates.SpaceRoles["admin"].TemplateRef, | ||
| CurrentBase1nsTemplates.SpaceRoles["edit"].TemplateRef: CurrentBase1nsTemplates.SpaceRoles["edit"].TemplateRef, | ||
| CurrentBase1nsTemplates.SpaceRoles["viewer"].TemplateRef: CurrentBase1nsTemplates.SpaceRoles["viewer"].TemplateRef, |
There was a problem hiding this comment.
would be nice to have different values for the key and value to simulate the end-goal where the values will contain names of the TTR CRs
|
|
@fbm3307: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR is to update the space controller to compute the tier-hash label value using the NSTemplateTier.Status.Revisions field so that it can match outdated spaces using the new revisions mechanism.
Jira-Link - KUBESAW-147
Related PR
Paired PR