-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setup Basic Tests for Ceph-CSI-Operator #74
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Divyansh Kamboj <[email protected]>
k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) | ||
Expect(err).NotTo(HaveOccurred()) | ||
|
||
clientset, err = kubernetes.NewForConfig(cfg) | ||
Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are e2e tests, we should use exec.Command
instead of a client
BeforeAll(func() { | ||
By("installing prometheus operator") | ||
Expect(utils.InstallPrometheusOperator()).To(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prom is very useful in e2e testing. why drop it?
By("uninstalling the Prometheus manager bundle") | ||
utils.UninstallPrometheusOperator() | ||
|
||
By("uninstalling the cert-manager bundle") | ||
utils.UninstallCertManager() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prom is very useful in e2e testing. why drop it?
//nolint:gosec | ||
cmd := exec.Command("kubectl", "delete", "ns", namespace) | ||
_, _ = utils.Run(cmd) | ||
ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} | ||
err := k8sClient.Delete(context.TODO(), ns) | ||
Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are e2e tests, we should use exec.Command instead of a client
ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change, the withOffest
part is crucial here to make sense of error message
cmd = exec.Command("make", "install") | ||
_, err = utils.Run(cmd) | ||
ExpectWithOffset(1, err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change, the withOffest
part is crucial here to make sense of error message
ExpectWithOffset(1, err).NotTo(HaveOccurred()) | ||
|
||
By("validating that the controller-manager pod is running as expected") | ||
verifyControllerUp := func() error { | ||
// Get pod name | ||
//nolint:gosec | ||
cmd = exec.Command("kubectl", "get", | ||
"pods", "-l", "control-plane=controller-manager", | ||
"-o", "go-template={{ range .items }}"+ | ||
"{{ if not .metadata.deletionTimestamp }}"+ | ||
"{{ .metadata.name }}"+ | ||
"{{ \"\\n\" }}{{ end }}{{ end }}", | ||
"-n", namespace, | ||
) | ||
|
||
podOutput, err := utils.Run(cmd) | ||
ExpectWithOffset(2, err).NotTo(HaveOccurred()) | ||
podNames := utils.GetNonEmptyLines(string(podOutput)) | ||
if len(podNames) != 1 { | ||
return fmt.Errorf("expect 1 controller pods running, but got %d", len(podNames)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the operator deployment verification?
pods, err := clientset.CoreV1().Pods(namespace).List(context.TODO(), metav1.ListOptions{ | ||
LabelSelector: "control-plane=ceph-csi-op-controller-manager", | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Command.exec for this
driver := &csiv1alpha1.Driver{ | ||
TypeMeta: metav1.TypeMeta{ | ||
APIVersion: "csi.ceph.io/v1alpha1", | ||
Kind: "Driver", | ||
}, | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "test.rbd.csi.ceph.com", | ||
Namespace: namespace, | ||
}, | ||
Spec: csiv1alpha1.DriverSpec{ | ||
AttachRequired: ptr.To(true), | ||
NodePlugin: &csiv1alpha1.NodePluginSpec{}, | ||
ControllerPlugin: &csiv1alpha1.ControllerPluginSpec{}, | ||
}, | ||
} | ||
EventuallyWithOffset(1, verifyControllerUp, time.Minute, time.Second).Should(Succeed()) | ||
|
||
By("creating a Driver CR") | ||
Expect(k8sClient.Create(context.TODO(), driver)).To(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not unit tests these are e2e tests and should be code from a kubectl perspective
please use Command.Exec and a fixed file to read from
daemonset := &appsv1.DaemonSet{} | ||
err := k8sClient.Get(context.TODO(), client.ObjectKey{Name: "test.rbd.csi.ceph.com-nodeplugin", Namespace: namespace}, daemonset) | ||
if err != nil { | ||
return err | ||
} | ||
if daemonset.Status.NumberReady != daemonset.Status.DesiredNumberScheduled { | ||
return fmt.Errorf("daemonset not ready, expected %d nodes, got %d", daemonset.Status.DesiredNumberScheduled, daemonset.Status.NumberReady) | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify using kubectl
and command.Exec
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions. |
This PR introduces the initial setup for basic tests for the Ceph-CSI-Operator. It lays the groundwork for basic end to end tests for the operator
Describe what this PR does
Provide some context for the reviewer
Is there anything that requires special attention
Do you have any questions?
Is the change backward compatible?
Are there concerns around backward compatibility?
Provide any external context for the change, if any.
For example:
Related issues
Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.
Fixes: #issue_number
Future concerns
List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.
Checklist:
guidelines in the developer
guide.
Request
notes
updated with breaking and/or notable changes for the next major release.