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

Update NetworkAddonsConfig Status field with conditions #29

Merged
merged 7 commits into from
Apr 8, 2019
Merged

Update NetworkAddonsConfig Status field with conditions #29

merged 7 commits into from
Apr 8, 2019

Conversation

phoracek
Copy link
Member

@phoracek phoracek commented Apr 2, 2019

With this PR we update NetworkAddonsConfig Status field with set of
conditions. These conditions include Failing, Progressing and Ready.

Failing indicates error either on rendering/applying level of
NetworkAddonsConfig operator of an error that happened during deployment
of components's pods.

Progressing indicates on-going deployment of components.

Ready set to True indicates that all components has been successfully
deployed.

Failing Status field:

status:
  conditions:
  - lastProbeTime: 2019-04-02T14:26:54Z
    lastTransitionTime: 2019-04-02T14:26:51Z
    message: |-
      invalid configuration:
      endPoolRange must be configured
      requested imagePullPolicy 'Alwayss' is not valid
    reason: FailedToRender
    status: "True"
    type: Failing

Progressing Status field:

status:
  conditions:
  - lastProbeTime: 2019-04-02T14:28:17Z
    lastTransitionTime: 2019-04-02T14:28:17Z
    status: "False"
    type: Failing
  - lastProbeTime: 2019-04-02T14:28:44Z
    lastTransitionTime: 2019-04-02T14:28:44Z
    message: |-
      DaemonSet "linux-bridge/kube-cni-linux-bridge-plugin" is not available (awaiting 1 nodes)
      DaemonSet "sriov/kube-sriov-cni-plugin" is not available (awaiting 1 nodes)
      DaemonSet "sriov/kube-sriov-device-plugin" is not available (awaiting 1 nodes)
    reason: Deploying
    status: "True"
    type: Progressing
  - lastProbeTime: 2019-04-02T14:28:44Z
    lastTransitionTime: 2019-04-02T14:28:44Z
    message: Configuration is in process
    reason: Startup
    status: "False"
    type: Ready

Ready Status field:

status:
  conditions:
  - lastProbeTime: 2019-04-02T14:28:17Z
    lastTransitionTime: 2019-04-02T14:28:17Z
    status: "False"
    type: Failing
  - lastProbeTime: 2019-04-02T14:29:27Z
    lastTransitionTime: 2019-04-02T14:29:27Z
    status: "False"
    type: Progressing
  - lastProbeTime: 2019-04-02T14:29:27Z
    lastTransitionTime: 2019-04-02T14:29:27Z
    status: "True"
    type: Ready

@phoracek phoracek self-assigned this Apr 2, 2019
@phoracek phoracek requested review from booxter and SchSeba April 2, 2019 14:35
@phoracek
Copy link
Member Author

phoracek commented Apr 2, 2019

@SchSeba @booxter please take a look. I recommend reviewing it commit by commit.

@phoracek phoracek mentioned this pull request Apr 2, 2019
Closed
@phoracek
Copy link
Member Author

phoracek commented Apr 3, 2019

/hold

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2019
@phoracek
Copy link
Member Author

phoracek commented Apr 3, 2019 •
edited

Need to add monitoring of StatefulSet used by kubemacpool. I also think that there is an issue that we show status as progressing only after the first pod is successfully created.

@phoracek
Copy link
Member Author

phoracek commented Apr 3, 2019

I resolved the reporting problem. Now we need to find out whether we can use Deployment for kubemacpool or we need to add monitoring for StatefulSet.

@phoracek phoracek added this to the v0.3.0 milestone Apr 4, 2019
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2019
SchSeba
SchSeba approved these changes Apr 8, 2019
View reviewed changes
Copy link
Contributor

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

LGTM

just a small nit

pkg/controller/networkaddonsconfig/pod_controller.go Outdated Show resolved Hide resolved
@ovirt-infra
Copy link

All tests passed

booxter
booxter reviewed Apr 8, 2019
View reviewed changes
pkg/controller/networkaddonsconfig/networkaddonsconfig_controller.go Outdated
@@ -55,17 +56,18 @@ func Add(mgr manager.Manager) error {
}

// newReconciler returns a new reconcile.Reconciler
Copy link
Contributor

Choose a reason for hiding this comment

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

should this comment be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

booxter
booxter reviewed Apr 8, 2019
View reviewed changes
pkg/controller/networkaddonsconfig/networkaddonsconfig_controller.go Outdated
return &ReconcileNetworkAddonsConfig{
client: mgr.GetClient(),
scheme: mgr.GetScheme(),
podReconciler: newPodReconciler(),
sccIsAvailable: sccIsAvailable,
}
}

// add adds a new Controller to mgr with r as the reconcile.Reconciler
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@booxter
Copy link
Contributor

booxter commented Apr 8, 2019 •
edited

I can only applaud to the clarity of the code, all the comments describing the logic, the way PR is split into commits etc. Great stuff.

@phoracek
Copy link
Member Author

phoracek commented Apr 8, 2019 •
edited

😊 thanks

@phoracek
add status structures to api
74cea49
@phoracek
set daemonsets' apiVersion to apps/v1
54ad0a1
@phoracek
add pods reconcilier
74d0fe4 
This PR add a reconciliation for pods. This will be later used to
update state based on components' pods.
@phoracek
split Reconcile into multiple functions
9a8393a 
In order to make code more readable and prepare it for status managing,
we split Reconcile method of NetworkAddonsConfig operator into subfunctions.
@phoracek
update status field with conditions
78644eb 
This patch adds StatusManager which is responsible of updating
NetworkAddonsConfig Status field with set of conditions. These conditions
include Failing, Progressing and Ready.

Failing indicates error either on rendering/applying level of
NetworkAddonsConfig operator of an error that happened during deployment
of components's pods.

Progressing indicates on-going deployment of components.

Ready set to True indicates that all components has been successfully
deployed.
@phoracek
show rendering errors in condition line by line
b51779c
@phoracek
update README with conditions improvements
1e2c3fc
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2019
@phoracek phoracek merged commit af73054 into kubevirt:master Apr 8, 2019
@ovirt-infra
Copy link

Some tests failed

@phoracek phoracek mentioned this pull request Apr 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@booxter booxter booxter approved these changes

@SchSeba SchSeba SchSeba approved these changes

Assignees

@phoracek phoracek

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XL
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@phoracek @ovirt-infra @booxter @SchSeba @kubevirt-bot