Skip to content
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

Basic E2E test for machine controller #80

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Basic E2E test for machine controller #80

merged 2 commits into from
Feb 8, 2024

Conversation

mhmxs
Copy link
Contributor

@mhmxs mhmxs commented Feb 5, 2024

At the time of this PR repo secrets are not available on forked repos, so this PR is a replication of #71

Some small bugfixes came around the test execution itself, so business logic didn't change compare to the original PR.

@mhmxs mhmxs force-pushed the e2e-test-machine branch 13 times, most recently from 729de3e to e49659e Compare February 6, 2024 11:38

return err
}
createConfig.Label = machineScope.LinodeMachine.Spec.Label
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AshleyDumaine linodeMachineSpecToInstanceCreateConfig set the label, we don't have to set it manually. We have test for it: https://github.com/linode/cluster-api-provider-linode/blob/main/controller/linodemachine_controller_helpers_test.go#L22 so there must be an error is it is empty after the conversion.

@mhmxs
Copy link
Contributor Author

mhmxs commented Feb 7, 2024

Depends on: #86

commands:
- script: |-
MACHINE_UID="$(OBJ=machines/machine-sample make getKubeUid)" \
TC="$(TPL="$PWD/02-create-linodemachine.tpl.yml" make renderTestCase)" make runTestCase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While running the e2etest target locally, kuttl failed at this step:

=== CONT  kuttl/harness/minimal#02
    logger.go:42: 13:00:28 | minimal/1-create-cluster | LinodeCluster:kuttl-test-topical-moray/linodecluster-sample created
    logger.go:42: 13:00:28 | minimal/1-create-cluster | Cluster:kuttl-test-topical-moray/cluster-sample created
    logger.go:42: 13:00:28 | minimal/1-create-cluster | Machine:kuttl-test-topical-moray/machine-sample created
    logger.go:42: 13:00:28 | minimal/1-create-cluster | Secret:kuttl-test-topical-moray/bootstrap-data-sample created
    logger.go:42: 13:00:28 | minimal/1-create-cluster | test step completed 1-create-cluster
    logger.go:42: 13:00:28 | minimal/2-create-linodemachine | starting test step 2-create-linodemachine
    logger.go:42: 13:00:28 | minimal/2-create-linodemachine | running command: [sh -c MACHINE_UID="$(OBJ=machines/machine-sample make getKubeUid)" \
        TC="$(TPL="$PWD/02-create-linodemachine.tpl.yml" make renderTestCase)" make runTestCase]
    logger.go:42: 13:00:28 | minimal/2-create-linodemachine | Error: no test directories provided, please provide either --config or test directories on the command line
    logger.go:42: 13:00:28 | minimal/2-create-linodemachine | make[2]: *** [runTestCase] Error 1
    case.go:364: failed in step 2-create-linodemachine
    case.go:366: exit status 2
    logger.go:42: 13:00:28 | minimal | minimal events from ns kuttl-test-topical-moray:
    logger.go:42: 13:00:28 | minimal | Deleting namespace: kuttl-test-topical-moray

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcm820

You can test rendering part with this commands, maybe it helps find the problem:

e2e/linodemachine-controller/minimal
❯ TPL=02-create-linodemachine.tpl.yml make renderTestCase 
/run/user/51170/tmp.1GKQUKF99C
e2e/linodemachine-controller/minimal
❯ cat /run/user/51170/tmp.1GKQUKF99C/step/00-step.yaml 
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeMachine
metadata:
  ownerReferences:
  - apiVersion: cluster.x-k8s.io/v1beta1
    kind: Machine
    name: machine-sample
    uid: 
  name: linodemachine-sample
spec:
  region: us-sea
  type: g5-nanode-1

@@ -294,7 +294,7 @@ func (r *LinodeMachineReconciler) reconcileCreate(
logger logr.Logger,
machineScope *scope.MachineScope,
clusterScope *scope.ClusterScope,
) error {
) (*linodego.Instance, error) {
Copy link
Contributor Author

@mhmxs mhmxs Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AshleyDumaine there is a defer function which updates the status based on the linode instance status, so in this case we need the instance itself. This changes fixes: c2b2378#diff-8daab1045cecf480419b0a31c2f8558e51197d31a7be910386ef4e420db0f7a4

@mhmxs
Copy link
Contributor Author

mhmxs commented Feb 8, 2024

@AshleyDumaine @avestuk @eljohnson92 Could you please take a final look. I fixed a bug which was introduced on main after your review.

@mhmxs mhmxs merged commit c659027 into main Feb 8, 2024
6 checks passed
@mhmxs mhmxs deleted the e2e-test-machine branch February 8, 2024 14:35
@D="$$(mktemp -d)" ;\
mkdir -p "$$D/step" ;\
envsubst -i "$$TPL" -o "$$D/step/00-step.yaml" ;\
echo -n "$$D"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I print the value of TC, it prints as -n /var/folders/c3/y7njgw_x373gsj4nydxmjgz40000gq/T/tmp.af02uAbM (treating -n as a literal). `echo "$$D\c" worked fine here as a replacement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I print the value of TC, it prints as -n /var/folders/c3/y7njgw_x373gsj4nydxmjgz40000gq/T/tmp.af02uAbM (treating -n as a literal). `echo "$$D\c" worked fine here as a replacement.

@bcm820 What kind of OS and shell do you use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhmxs - I didn't see that you merged already. I'll put up a separate PR.

Copy link
Contributor

@bcm820 bcm820 Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use a Mac. By default, kuttl uses sh native to the OS, and Mac's builtin echo does not support passing -n.

amold1 pushed a commit that referenced this pull request May 17, 2024
* Basic E2E test for machine controller

* Fix linode machine update

---------

Co-authored-by: Richard Kovacs <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants