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.

Sign up for GitHub

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

Jump to bottom

🌱 Add conditions to the DockerMachine object #3122

Merged
merged 1 commit into from
Jun 4, 2020
Merged

🌱 Add conditions to the DockerMachine object #3122

merged 1 commit into from
Jun 4, 2020

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR adds conditions to the DockerMachine object

Which issue(s) this PR fixes:
Fixes #3111

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 1, 2020
@k8s-ci-robot k8s-ci-robot requested review from detiber and ncdc June 1, 2020 11:55
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2020
@fabriziopandini fabriziopandini force-pushed the add-conditions-to-dockermachine branch 2 times, most recently from a8c06b6 to 6f60bd3 Compare June 1, 2020 12:47
detiber
detiber reviewed Jun 1, 2020
View reviewed changes
test/infrastructure/docker/controllers/dockermachine_controller.go Outdated Show resolved Hide resolved
test/infrastructure/docker/controllers/dockermachine_controller.go Outdated
@@ -209,34 +232,48 @@ func (r *DockerMachineReconciler) reconcileNormal(ctx context.Context, machine *
dockerMachine.Status.LoadBalancerConfigured = true
}

// At, this stage, we are ready for bootstrap. However, if the BootstrapExecSucceededCondition is missing we add it and we
// requeue so the user can se the change of state before the bootstrap actually start.
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to do this here? Most (if not all) other infrastructure providers do not have a differentiation between provisioning the infrastructure and bootstrapping since it is done through setting user data (or metadata) on the instance. We only have a distinction here because we are simulating cloud-init.

I think having the separate conditions for the individual steps for this bootstrap provider is beneficial for troubleshooting/debugging purposes, but I don't see any particular reason that we should force a requeue here just to enable external tooling to trigger on provisioning happening prior to bootstrapping occuring.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, the main reason for this is that it makes a nicer UX when working with CAPD, because this is where the docker-machine provisioning spends most of the time.

test/infrastructure/docker/controllers/dockermachine_controller.go Show resolved Hide resolved
test/infrastructure/docker/controllers/dockermachine_controller.go Show resolved Hide resolved
test/infrastructure/docker/controllers/dockermachine_controller.go Outdated Show resolved Hide resolved
@vincepri
Copy link
Member

vincepri commented Jun 1, 2020

/retitle 🏃 Add conditions to the DockerMachine object

@k8s-ci-robot k8s-ci-robot changed the title ✨ Add conditions to the DockerMachine object 🏃 Add conditions to the DockerMachine object Jun 1, 2020
@vincepri
Copy link
Member

vincepri commented Jun 2, 2020

/retitle 🌱 Add conditions to the DockerMachine object

@k8s-ci-robot k8s-ci-robot changed the title 🏃 Add conditions to the DockerMachine object 🌱 Add conditions to the DockerMachine object Jun 2, 2020
@fabriziopandini fabriziopandini mentioned this pull request Jun 3, 2020
Closed
vincepri
vincepri approved these changes Jun 3, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/approve
/assign @detiber
/milestone v0.3.7

@k8s-ci-robot k8s-ci-robot added this to the v0.3.7 milestone Jun 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fabriziopandini
Copy link
Member Author

I'm going to remove ImageLoaded flag...

@fabriziopandini
Copy link
Member Author

@detiber flag removed (last commit)
Let me know if I can squash commits or there is something else to be addressed

@detiber
Copy link
Member

detiber commented Jun 3, 2020

lgtm pending squash

@fabriziopandini fabriziopandini force-pushed the add-conditions-to-dockermachine branch from 8559e7a to 166e106 Compare June 3, 2020 19:46
@fabriziopandini
Copy link
Member Author

@detiber re-based

@vincepri
Copy link
Member

vincepri commented Jun 4, 2020

LGTM

detiber
detiber reviewed Jun 4, 2020
View reviewed changes
test/infrastructure/docker/controllers/dockermachine_controller.go Outdated
}

timeoutctx, cancel := context.WithTimeout(ctx, 3*time.Minute)
defer cancel()
// Run the bootstrap script. Simulates cloud-init.
if err := externalMachine.ExecBootstrap(timeoutctx, bootstrapData); err != nil {
return ctrl.Result{}, errors.Wrap(err, "failed to exec DockerMachine bootstrap")
conditions.MarkFalse(dockerMachine, infrav1.BootstrapExecSucceededCondition, infrav1.BootstrapFailedReason, clusterv1.ConditionSeverityWarning, "Repeating bootstrap")
return ctrl.Result{RequeueAfter: 5 * time.Second}, errors.Wrap(err, "failed to exec DockerMachine bootstrap")
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that this is setting both RequeueAfter and returning an error, the RequeueAfter will be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@fabriziopandini fabriziopandini force-pushed the add-conditions-to-dockermachine branch from 166e106 to 48f2010 Compare June 4, 2020 16:36
@fabriziopandini
Copy link
Member Author

@detiber PTAL

@detiber
Copy link
Member

detiber commented Jun 4, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2020
@k8s-ci-robot k8s-ci-robot merged commit e72c3ae into kubernetes-sigs:master Jun 4, 2020
@fabriziopandini fabriziopandini deleted the add-conditions-to-dockermachine branch June 9, 2020 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmoss benmoss benmoss left review comments

@detiber detiber detiber left review comments

@vincepri vincepri vincepri approved these changes

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.7
Development

Successfully merging this pull request may close these issues.

Add conditions to the DockerMachine object
5 participants
@fabriziopandini @vincepri @k8s-ci-robot @detiber @benmoss