bug: Apply the NFS-CSI namespace first thing

[aerosouund]

What this PR does / why we need it:

Walking the directory results in things that depend on the namespace to be created first and provisioning fails. This ensures the namespace is the first thing created.
Allow more time for the test volume to become ready.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Design: A design document was considered and is present (link) or not required
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• Community: Announcement to kubevirt-dev was considered

[kubevirt-bot]

Hi @aerosouund. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

[dhiller]

/test all

[xpivarc]

/retest-required

[brianmcarey]

/test all

[dhiller]

Sorry for spotting this so late - I think there's a fundamental problem with the approach of creating the kubernetes objects here which is now getting obvious: since we are parsing the yaml and trying to create the objects one by one we are running into the problem of creation failing when a required object is not already there.

Before we were just kubectl apply -f ing the yamls and the kubernetes server was figuring out what to do, I believe that it should now work in the same way as before.

Another problem is related to updates of the manifests: if we are splitting the original yamls we are complicating things for people trying to apply updates to them, since they need to re-split them and adjust them - before they could just throw them into the folder and the update would be done. This also should work as it did before.

Please rewrite the k8s object creation in sending the yamls to the kubernetes api.

One other thing: if you are fixing a bug, please add a reference to the PR description so that reviewers can see exactly what the problem is.

Thank you for your contribution!

[aerosouund]

@dhiller
highly agree on the part of our current approach needing improvement. however i believe that in the original bash approach the reason why it worked because the lines were arranged properly in terms of what depends on what.

e.g:

# CRDS
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/snapshot.storage.k8s.io_volumesnapshots.yaml
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/snapshot.storage.k8s.io_volumesnapshotcontents.yaml
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

# CLUSTER ROLES
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/rbac-snapshot-controller.yaml
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/setup-snapshot-controller.yaml

# NAMESPACE
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/common.yaml

# MORE CRDS
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/crds.yaml

# DEPENDENT RESOURCES
kubectl --kubeconfig /etc/kubernetes/admin.conf create -f /tmp/ceph/operator.yaml

And this was transferred to the original go implementation

	manifests := []string{
		"manifests/snapshot.storage.k8s.io_volumesnapshots.yaml",
		"manifests/snapshot.storage.k8s.io_volumesnapshotcontents.yaml",
		"manifests/snapshot.storage.k8s.io_volumesnapshotclasses.yaml",
		"manifests/rbac-snapshot-controller.yaml",
		"manifests/setup-snapshot-controller.yaml",
		"manifests/common.yaml",
		"manifests/crds.yaml",
		"manifests/operator.yaml",
		"manifests/cluster-test.yaml",
		"manifests/pool-test.yaml",
	}

	for _, manifest := range manifests {
		yamlData, err := f.ReadFile(manifest)
		if err != nil {
			return err
		}
		if err := o.client.Apply(yamlData); err != nil {
			return err
		}
	}

But since we changed it to this

	err := fs.WalkDir(f, "manifests", func(path string, d fs.DirEntry, err error) error {
		if err != nil {
			return err
		}
		if !d.IsDir() && filepath.Ext(path) == ".yaml" {
			yamlData, err := f.ReadFile(path)
			if err != nil {
				return err
			}
			yamlDocs := bytes.Split(yamlData, []byte("---\n"))
			for _, yamlDoc := range yamlDocs {
				if len(yamlDoc) == 0 {
					continue
				}

				obj, err := k8s.SerializeIntoObject(yamlDoc)
				if err != nil {
					logrus.Info(err.Error())
					continue
				}
				if err := o.client.Apply(obj); err != nil {
					return fmt.Errorf("error applying manifest %s", err)
				}
			}
		}
		return nil
	})

We lost the ordering of the manifests. I apologize for not having noticed this myself either since i depended on the unit test to validate the change. but the unit test uses the fake kubernetes client which doesn't preform this dependency check

	var (
		k8sClient k8s.K8sDynamicClient
		opt       *cephOpt
	)

	BeforeEach(func() {
		r := k8s.NewReactorConfig("create", "cephblockpools", CephReactor)
		k8sClient = k8s.NewTestClient(r)
		opt = NewCephOpt(k8sClient)
	})

	It("should execute NfsCsiOpt successfully", func() {
		err := opt.Exec()
		Expect(err).NotTo(HaveOccurred())
	})

And so i suggest we go back to the original implementation as a potential solution

[aerosouund]

Another problem is related to updates of the manifests: if we are splitting the original yamls we are complicating things for people trying to apply updates to them, since they need to re-split them and adjust them - before they could just throw them into the folder and the update would be done. This also should work as it did before.

Happy to enhance this part as well, but can you please clarify more ? because the splitting doesn't happen at the level of how we store the manifests, it happens during execution of an apply call. so its perfectly safe to group multiple manifests as one without worrying about how they are stored.

The reason why i did the change of grouping the ceph crds is to avoid having to embed every single CRD file and applying it before the rest of the resources, just from a code cleanliness perspective. but it was perfectly valid to keep them as they are

[dhiller]

Another problem is related to updates of the manifests: if we are splitting the original yamls we are complicating things for people trying to apply updates to them, since they need to re-split them and adjust them - before they could just throw them into the folder and the update would be done. This also should work as it did before.

Happy to enhance this part as well, but can you please clarify more ? because the splitting doesn't happen at the level of how we store the manifests, it happens during execution of an apply call. so its perfectly safe to group multiple manifests as one without worrying about how they are stored.

The reason why i did the change of grouping the ceph crds is to avoid having to embed every single CRD file and applying it before the rest of the resources, just from a code cleanliness perspective. but it was perfectly valid to keep them as they are

I was referring to this PR - in this you extract the namespace definition from the original yaml, which is a modification that people trying to update the target manifest need to reapply.

[aerosouund]

@dhiller
I have further findings about this
I was curious as to why this issue appeared for ceph and nfs csi only, while it should have also appeared in prometheus and CNAO

As it turns out this code

	err := fs.WalkDir(f, "manifests", func(path string, d fs.DirEntry, err error)

will not return a random order of the files, but rather the files sorted in alphabetical order.
And so we can understand why it didn't happen for prometheus and CNAO, since the namespace file in prometheus is placed in 0namespace-namespace.yaml and for CNAO:

crd.yaml              
namespace.yaml
network-addons-config-example.cr.yaml  
operator.yaml    
whereabouts.yaml                  

all the dependent resources have names that starts with letters after n.
And so another solution is to name the file containing the namespace with a 0 in the beginning or something similar.
Doing this means we don't need to order the manifests, and we don't need to embed and apply single manifests before others (the solution that this PR implements)

Let me know what you think

[aerosouund]

I was referring to this PR - in this you extract the namespace definition from the original yaml, which is a modification that people trying to update the target manifest need to reapply.

I see, well. based on the findings i shared some form of manifests ordering is needed. Let me know which approach works best
in my opinion the approach of renaming the namespace file to have a 0 in the beginning is the best since it allows us to keep all the benefits accrued from walking the directory

[dhiller]

@dhiller I have further findings about this I was curious as to why this issue appeared for ceph and nfs csi only, while it should have also appeared in prometheus and CNAO

As it turns out this code

	err := fs.WalkDir(f, "manifests", func(path string, d fs.DirEntry, err error)

will not return a random order of the files, but rather the files sorted in alphabetical order.

Great to hear that it's deterministic 😄

And so we can understand why it didn't happen for prometheus and CNAO, since the namespace file in prometheus is placed in 0namespace-namespace.yaml and for CNAO

crd.yaml              
namespace.yaml
network-addons-config-example.cr.yaml  
operator.yaml    
whereabouts.yaml                  

all the dependent resources have names that starts with letters after n. And so another solution is to name the file containing the namespace with a 0 in the beginning or something similar. Doing this means we don't need to order the manifests, and we don't need to embed and apply single manifests before others (the solution that this PR implements)

Let me know what you think

Sounds good to me.

[aerosouund]

Awesome, i'll push code shortly