feat: create tiertemplaterevision for each tiertemplate #1061

mfrancisc · 2024-11-12T08:55:08Z

E2e tests for: codeready-toolchain/host-operator#1103

verifies that when creating a TierTemplate with the TemplateObjects field populated it triggers the creation of a TierTemplateRevision CR as well
verify owner reference of the TTR
verify labels are set correctly on the TTR

Jira: https://issues.redhat.com/browse/KUBESAW-146

# Conflicts: # testsupport/init.go

test/e2e/parallel/nstemplatetier_test.go

testsupport/wait/host.go

test/e2e/parallel/nstemplatetier_test.go

mfrancisc · 2024-11-20T09:17:52Z

seems like some infra/flaky issue:

  proxy_test.go:385: 
        	Error Trace:	/go/src/github.com/codeready-toolchain/toolchain-e2e/test/e2e/parallel/proxy_test.go:385
        	Error:      	Received unexpected error:
        	            	configmaps "kube-root-ca.crt" not found

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/codeready-toolchain_toolchain-e2e/1061/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1859128319136501760/build-log.txt

mfrancisc · 2024-11-20T09:18:09Z

/retest

mfrancisc · 2024-11-20T10:43:36Z

infra issues:

WARNING: timed out waiting for git server, will wait 1m4s
error: fatal: unable to access 'https://github.com/codeready-toolchain/toolchain-e2e.git/': Could not resolve host: github.com

/retest

rsoaresd

Good job! 🚀 Just a minor question:

testsupport/wait/host.go

mfrancisc · 2024-11-26T18:26:10Z

/retest

user management test failure: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/codeready-toolchain_toolchain-e2e/1061/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1861390378251325440/build-log.txt looking into it to see if it's flaky

test/e2e/parallel/nstemplatetier_test.go

mfrancisc · 2024-12-05T17:07:33Z

/retest

openshift-ci · 2024-12-12T09:07:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: metlos, mfrancisc

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~~ [metlos,mfrancisc]

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

sonarqubecloud · 2024-12-19T09:10:35Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

MatousJobanek · 2024-12-20T16:19:11Z

test/e2e/parallel/nstemplatetier_test.go

+	namespaceResourcesWithTemplateObjects := tiers.WithNamespaceResources(t, baseTier, updateTierTemplateObjects)
+	clusterResourcesWithTemplateObjects := tiers.WithClusterResources(t, baseTier, updateTierTemplateObjects)
+	spaceRolesWithTemplateObjects := tiers.WithSpaceRoles(t, baseTier, updateTierTemplateObjects)
+	tiers.CreateCustomNSTemplateTier(t, hostAwait, "ttr", baseTier, namespaceResourcesWithTemplateObjects, clusterResourcesWithTemplateObjects, spaceRolesWithTemplateObjects, tiers.WithParameter("DEPLOYMENT_QUOTA", "60"))


easier to read:

Suggested change

namespaceResourcesWithTemplateObjects := tiers.WithNamespaceResources(t, baseTier, updateTierTemplateObjects)

clusterResourcesWithTemplateObjects := tiers.WithClusterResources(t, baseTier, updateTierTemplateObjects)

spaceRolesWithTemplateObjects := tiers.WithSpaceRoles(t, baseTier, updateTierTemplateObjects)

tiers.CreateCustomNSTemplateTier(t, hostAwait, "ttr", baseTier, namespaceResourcesWithTemplateObjects, clusterResourcesWithTemplateObjects, spaceRolesWithTemplateObjects, tiers.WithParameter("DEPLOYMENT_QUOTA", "60"))

tiers.CreateCustomNSTemplateTier(t, hostAwait, "ttr", baseTier,

tiers.WithNamespaceResources(t, baseTier, updateTierTemplateObjects),

tiers.WithClusterResources(t, baseTier, updateTierTemplateObjects),

tiers.WithSpaceRoles(t, baseTier, updateTierTemplateObjects),

tiers.WithParameter("DEPLOYMENT_QUOTA", "60"))

and btw, the CRQ logically doesn't make much sense for the namespace-scoped templates (which is namespace and spaceRole templates) but I understand that this is only for the testing purposes.
Let's maybe make a comment and make cure that we don't try to provision a user with it.

MatousJobanek · 2024-12-20T16:26:00Z

test/e2e/parallel/nstemplatetier_test.go

+	customTier, err := hostAwait.WaitForNSTemplateTierAndCheckTemplates(t, "ttr", wait.HasStatusTierTemplateRevisions([]string{
+		fmt.Sprintf("ttrfrom%s", baseTier.Spec.Namespaces[0].TemplateRef),       // check that the revision field is set using the expected tierTemplate refs as keys
+		fmt.Sprintf("ttrfrom%s", baseTier.Spec.SpaceRoles["admin"].TemplateRef), // we can safely use the template refs from base tier since the custom tier was created from base one.
+		fmt.Sprintf("ttrfrom%s", baseTier.Spec.ClusterResources.TemplateRef),
+	}))


why not using tiers.GetTemplateRefs(t, hostAwait, "ttr").Flatten()? Wouldn't it have the same result?

Suggested change

customTier, err := hostAwait.WaitForNSTemplateTierAndCheckTemplates(t, "ttr", wait.HasStatusTierTemplateRevisions([]string{

fmt.Sprintf("ttrfrom%s", baseTier.Spec.Namespaces[0].TemplateRef), // check that the revision field is set using the expected tierTemplate refs as keys

fmt.Sprintf("ttrfrom%s", baseTier.Spec.SpaceRoles["admin"].TemplateRef), // we can safely use the template refs from base tier since the custom tier was created from base one.

fmt.Sprintf("ttrfrom%s", baseTier.Spec.ClusterResources.TemplateRef),

}))

customTier, err := hostAwait.WaitForNSTemplateTierAndCheckTemplates(t, "ttr", wait.HasStatusTierTemplateRevisions([]string{

fmt.Sprintf("ttrfrom%s", baseTier.Spec.Namespaces[0].TemplateRef), // check that the revision field is set using the expected tierTemplate refs as keys

customTier, err := hostAwait.WaitForNSTemplateTierAndCheckTemplates(t, "ttr",

wait.HasStatusTierTemplateRevisions(tiers.GetTemplateRefs(t, hostAwait, "ttr").Flatten()))

MatousJobanek · 2024-12-20T16:29:35Z

test/e2e/parallel/nstemplatetier_test.go

+		// But since the creation of a TTR could be very quick and could trigger another reconcile of the NSTemplateTier before the status is actually updated with the reference,
+		// this might generate some copies of the TTRs. This is not a problem in production since the cleanup mechanism of TTRs will remove the extra ones but could cause some flakiness with the test,
+		// thus we assert the number of TTRs doesn't exceed the double of the expected number.
+		assert.LessOrEqual(t, len(objs.Items), 6)


I guess that you can also use the len of TemplateRef from tiers.GetTemplateRefs(t, hostAwait, "ttr").Flatten(), right?
Just in case the number of templateRefs changes in the base1ns tier

MatousJobanek · 2024-12-20T16:31:37Z

test/e2e/parallel/nstemplatetier_test.go

+			assert.Equal(t, obj.Spec.Parameters[0].Name, customTier.Spec.Parameters[0].Name)
+			assert.Equal(t, obj.Spec.Parameters[0].Value, customTier.Spec.Parameters[0].Value)


the same as in the other PR:

Suggested change

assert.Equal(t, obj.Spec.Parameters[0].Name, customTier.Spec.Parameters[0].Name)

assert.Equal(t, obj.Spec.Parameters[0].Value, customTier.Spec.Parameters[0].Value)

assert.ElementsMatch(t, customTier.Spec.Parameters, obj.Spec.Parameters)

MatousJobanek · 2024-12-20T16:49:26Z

testsupport/cleanup/clean.go

+		for i := range ttrs.Items {
+			ttr := ttrs.Items[i]
+			c.t.Logf("deleting also the related TTR %s", ttr.GetName())
+			if err := c.client.Delete(context.TODO(), &ttr, propagationPolicyOpts); err != nil {
+				if errors.IsNotFound(err) {
+					continue
+				}
+				c.t.Logf("problem with deleting the related TTR %s: %s", ttr.GetName(), err)
+				return false, err
+			}
+		}


you can simplify this by calling DeleteAllOf

Suggested change

for i := range ttrs.Items {

ttr := ttrs.Items[i]

c.t.Logf("deleting also the related TTR %s", ttr.GetName())

if err := c.client.Delete(context.TODO(), &ttr, propagationPolicyOpts); err != nil {

if errors.IsNotFound(err) {

continue

}

c.t.Logf("problem with deleting the related TTR %s: %s", ttr.GetName(), err)

return false, err

}

}

return false, c.client.DeleteAllOf(context.TODO(), &toolchainv1alpha1.TierTemplateRevision{},

client.InNamespace(nsTemplateTier.GetNamespace()),

client.MatchingLabels{toolchainv1alpha1.TierLabelKey: nsTemplateTier.GetName()})

MatousJobanek · 2024-12-20T16:50:26Z

testsupport/cleanup/clean.go

+	} else if len(ttrs.Items) > 0 {
+		// ttrs are still there
+		return false, nil
+	}
+	return true, nil


if the ttr.Items is empty, then the method returns a few lines earlier

Suggested change

} else if len(ttrs.Items) > 0 {

// ttrs are still there

return false, nil

}

return true, nil

}

// ttrs are still there

return false, nil

MatousJobanek · 2024-12-20T16:59:14Z

testsupport/wait/host.go

+		if tierTemplateNamespaces.Spec.TemplateObjects != nil {
+			ttrName, found := tier.Status.Revisions[tierTemplateNamespaces.GetName()]


this technically expects that if the TierTemplate of the NSTemplateTier supports the new way of defining templates, then the function WaitForNSTemplateTierAndCheckTemplates has to be called with wait.HasStatusTierTemplateRevisions; otherwise, it may result with flakiness.
it's fine for now, but it has to be changed/improved in the future

MatousJobanek · 2024-12-20T17:02:58Z

testsupport/wait/host.go

+			if !found {
+				return nil, fmt.Errorf("missing revision for TierTemplate %s in NSTemplateTier '%s'", tierTemplateNamespaces.GetName(), tier.Name)
+			}
+			if _, err := a.WaitForTierTemplateRevision(t, ttrName); err != nil {


I guess that you can use simply

Suggested change

if _, err := a.WaitForTierTemplateRevision(t, ttrName); err != nil {

if _, err := For(t, a.Awaitility, &toolchainv1alpha1.TierTemplateRevision{}).WithNameThat(ttrName); err != nil {

and drop completely the WaitForTierTemplateRevision function.

MatousJobanek · 2024-12-20T17:10:31Z

testsupport/wait/host.go

+		// if the tier template supports Tier Template Revisions then let's check those
+		if tierTemplateClusterResources.Spec.TemplateObjects != nil {
+			ttrName, found := tier.Status.Revisions[tierTemplateClusterResources.GetName()]
+			if !found {
+				return nil, fmt.Errorf("missing revision for TierTemplate %s in NSTemplateTier '%s'", tierTemplateClusterResources.GetName(), tier.Name)
+			}
+			if _, err := a.WaitForTierTemplateRevision(t, ttrName); err != nil {
+				return nil, err
+			}
+		}


this code looks like the same as previous one - why not moving it to a separate method:

func (a *HostAwaitility) checkTTR(t *testing.T, tier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) error { // if the tier template supports Tier Template Revisions then let's check those if tierTemplate.Spec.TemplateObjects != nil { ttrName, found := tier.Status.Revisions[tierTemplate.GetName()] if !found { return fmt.Errorf("missing revision for TierTemplate %s in NSTemplateTier '%s'", tierTemplate.GetName(), tier.Name) } if _, err := For(t, a.Awaitility, &toolchainv1alpha1.TierTemplateRevision{}).WithNameThat(ttrName); err != nil { return err } } return nil }

so you can then call

if err := a.checkTTR(t, tier, tierTemplateClusterResources); err != nil { return nil, err }

Devtools added 4 commits November 11, 2024 12:10

wip add ttr tests

fb8a690

test TierTemplate revisions are created when templateObjects are set

d66af28

check for owner refernces

62a1a83

fix ownership check

09fe90b

mfrancisc requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan and ranakan19 as code owners November 12, 2024 08:55

openshift-ci bot added the approved label Nov 12, 2024

mfrancisc mentioned this pull request Nov 12, 2024

feat: create tiertemplaterevision for each tiertemplate codeready-toolchain/host-operator#1103

Open

Devtools and others added 2 commits November 12, 2024 15:30

remove unused import

edcf467

Merge branch 'master' into createttr

6b4deb8

openshift-merge-robot added the needs-rebase label Nov 18, 2024

Merge branch 'master' into createttr

b9e5e84

# Conflicts: # testsupport/init.go

openshift-merge-robot removed the needs-rebase label Nov 18, 2024

Devtools and others added 2 commits November 18, 2024 11:53

make linter happy

e91f57c

Merge branch 'master' into createttr

7e03339

MatousJobanek reviewed Nov 19, 2024

View reviewed changes

Devtools added 2 commits November 20, 2024 01:12

add comments, check for ttr content, check revisions

7d34843

fix compilation

b826d56

remove owner ref check

9cb5461

mfrancisc requested a review from MatousJobanek November 20, 2024 14:04

rsoaresd reviewed Nov 20, 2024

View reviewed changes

testsupport/wait/host.go Outdated Show resolved Hide resolved

Devtools added 3 commits November 20, 2024 15:25

avoid initializing twice

b50289c

remove second declaration

bdba663

add ttr cleanup logic

b4f334d

Devtools and others added 6 commits November 21, 2024 19:52

fix linter

dc38c17

Merge branch 'master' into createttr

22f4e64

Merge branch 'master' into createttr

a0ec293

Merge branch 'master' into createttr

b1c59ab

relax number of ttr check

a61de18

fix test

7665e00

Merge branch 'master' into createttr

f73f65b

metlos approved these changes Nov 27, 2024

View reviewed changes

Devtools and others added 6 commits November 28, 2024 16:08

handle parameters in TierTemplateRevision

02797f9

Merge remote-tracking branch 'origin/createttr' into createttr

f9069f6

fix test

62ee614

Merge branch 'master' into createttr

1b44744

update api

aec5c1d

Merge remote-tracking branch 'origin/createttr' into createttr

d18c2a4

metlos reviewed Dec 4, 2024

View reviewed changes

test/e2e/parallel/nstemplatetier_test.go Show resolved Hide resolved

mfrancisc and others added 2 commits December 5, 2024 13:54

Merge branch 'master' into createttr

8260a86

add comment about param assertion

f818ea9

openshift-merge-robot added the needs-rebase label Dec 10, 2024

merge

3f36b81

openshift-merge-robot removed the needs-rebase label Dec 10, 2024

fix merge

7dd6053

Merge branch 'master' into createttr

f442e1b

MatousJobanek reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create tiertemplaterevision for each tiertemplate #1061

feat: create tiertemplaterevision for each tiertemplate #1061

mfrancisc commented Nov 12, 2024 •

edited

Loading

mfrancisc commented Nov 20, 2024

mfrancisc commented Nov 20, 2024

mfrancisc commented Nov 20, 2024

rsoaresd left a comment

mfrancisc commented Nov 26, 2024

mfrancisc commented Dec 5, 2024

openshift-ci bot commented Dec 12, 2024

sonarqubecloud bot commented Dec 19, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

MatousJobanek Dec 20, 2024

		assert.Equal(t, obj.Spec.Parameters[0].Name, customTier.Spec.Parameters[0].Name)
		assert.Equal(t, obj.Spec.Parameters[0].Value, customTier.Spec.Parameters[0].Value)

	assert.Equal(t, obj.Spec.Parameters[0].Name, customTier.Spec.Parameters[0].Name)
	assert.Equal(t, obj.Spec.Parameters[0].Value, customTier.Spec.Parameters[0].Value)
	assert.ElementsMatch(t, customTier.Spec.Parameters, obj.Spec.Parameters)

		if tierTemplateNamespaces.Spec.TemplateObjects != nil {
		ttrName, found := tier.Status.Revisions[tierTemplateNamespaces.GetName()]

	if _, err := a.WaitForTierTemplateRevision(t, ttrName); err != nil {
	if _, err := For(t, a.Awaitility, &toolchainv1alpha1.TierTemplateRevision{}).WithNameThat(ttrName); err != nil {

feat: create tiertemplaterevision for each tiertemplate #1061

Are you sure you want to change the base?

feat: create tiertemplaterevision for each tiertemplate #1061

Conversation

mfrancisc commented Nov 12, 2024 • edited Loading

mfrancisc commented Nov 20, 2024

mfrancisc commented Nov 20, 2024

mfrancisc commented Nov 20, 2024

rsoaresd left a comment

Choose a reason for hiding this comment

mfrancisc commented Nov 26, 2024

mfrancisc commented Dec 5, 2024

openshift-ci bot commented Dec 12, 2024

sonarqubecloud bot commented Dec 19, 2024

Quality Gate passed

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

mfrancisc commented Nov 12, 2024 •

edited

Loading