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 node-conditions to the MachineStatus #483

Merged
merged 2 commits into from
Aug 23, 2018
Merged

Add node-conditions to the MachineStatus #483

merged 2 commits into from
Aug 23, 2018

Conversation

hardikdr
Copy link
Member

@hardikdr hardikdr commented Aug 21, 2018 •
edited

What this PR does / why we need it: Based on the discussion in the last sig-call, adding node-conditions into Machine-status. Basically Machine-set controller will be able to understand the Machine-health from these conditions- few of them areDiskPressure, KubeleteReady, MemoryPressure and so on. Based on these conditions - Machineset can decide whether Machine . needs to be replaced/udpated - eg. DiskPressure from X minutes, KubeleteNotReady from Y minutes so on.
Refer: https://kubernetes.io/docs/concepts/architecture/nodes/#condition

Which issue(s) this PR fixes :
This PR will partially resolve- #47, though we need to rediscuss the current approach where machineset is pulling the node-object to understand the machine-health.

Special notes for your reviewer: Executed make generate, make build, to mention explicitly make depend is getting stuck[for long] at dep issues due to api-builder- so avoided that for now.

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Added node-conditions into api for MachineStatus.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2018
@roberthbailey
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 22, 2018
@roberthbailey roberthbailey self-assigned this Aug 22, 2018
roberthbailey
roberthbailey reviewed Aug 22, 2018
View reviewed changes
Copy link
Contributor

@roberthbailey roberthbailey left a comment

Choose a reason for hiding this comment

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

Some nits about the comments for the new field.

pkg/apis/cluster/v1alpha1/machine_types.go Outdated
@@ -141,6 +141,14 @@ type MachineStatus struct {
// Addresses is a list of addresses assigned to the machine. Queried from cloud provider, if available.
// +optional
Addresses []corev1.NodeAddress `json:"addresses,omitempty"`

// List of conditions synced from the node-conditions of the corresponding node-object.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "node conditions" (with a space)

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

pkg/apis/cluster/v1alpha1/machine_types.go Outdated

// List of conditions synced from the node-conditions of the corresponding node-object.
// Machine-controller would be responsible for keeping conditions up-to-date.
// Machine-set controller will be taking these conditions as a signal to decide if
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: MachineSet controller

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

pkg/apis/cluster/v1alpha1/machine_types.go Outdated
@@ -141,6 +141,14 @@ type MachineStatus struct {
// Addresses is a list of addresses assigned to the machine. Queried from cloud provider, if available.
// +optional
Addresses []corev1.NodeAddress `json:"addresses,omitempty"`

// List of conditions synced from the node-conditions of the corresponding node-object.
// Machine-controller would be responsible for keeping conditions up-to-date.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Machine controller is responsible ...

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

@roberthbailey
Copy link
Contributor

/hold

Adding a hold since this is an API change to discuss during the weekly meeting.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 22, 2018
@davidewatson
Copy link
Contributor

/lgtm. This can also be used to fix #253

@davidewatson davidewatson mentioned this pull request Aug 22, 2018
Closed
@roberthbailey
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2018
@roberthbailey
Copy link
Contributor

Leaving the hold for ~1 day as discussed during the WG call today.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardikdr, roberthbailey

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2018
@roberthbailey
Copy link
Contributor

It's been a day, removing the hold.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit 2d1ad4c into kubernetes-sigs:master Aug 23, 2018
@davidewatson davidewatson mentioned this pull request Oct 4, 2018
Closed
@hardikdr hardikdr mentioned this pull request Nov 2, 2018
Closed
6 tasks
@davidewatson davidewatson mentioned this pull request Jun 28, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberthbailey roberthbailey roberthbailey left review comments

@jessicaochen jessicaochen Awaiting requested review from jessicaochen

Assignees

@roberthbailey roberthbailey

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
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hardikdr @roberthbailey @davidewatson @k8s-ci-robot