Add nmstate handler daemonset to operator #89

sjpotter · 2019-04-24T16:50:52Z

per my email to kubevirt-dev, here's my work in progress to add the nmstate handler from kubernetes-nmstate repo to the operator.

still have to check into the manifests it builds to see if anything has to be added, but figure get the PR out early so general comments can be made if I didn't understand something of how its supposed to work.

ovirt-infra · 2019-04-24T16:50:54Z

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

Type ci test please on this PR to trigger automated tests for it.
Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow need to change the deployment command #3 option )
If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

SchSeba

Thanks for the contribution!

Just a few comments

data/nmstate/001-rbac

data/nmstate/002-nmstated.yaml

data/nmstate/001-rbac

data/nmstate/000-ns.yaml

data/nmstate/002-nmstated.yaml

pkg/components/components.go

pkg/network/nmstate.go

phoracek

Just a nit, thanks for pushing this PR. Other than those 2 comments it looks good.

data/nmstate/002-nmstated.yaml

pkg/network/nmstate.go

SchSeba

I have another request can you add some documentation in the README file about nmstate(you can take it from the nmstate-kubernetes git repo)

SchSeba

Looks good to me just a small nit and I think it is ready @phoracek what you think?

README.md

cluster/functest.sh

pkg/apis/networkaddonsoperator/v1alpha1/networkaddonsconfig_types.go

SchSeba

LGTM!
@phoracek you have any other request?

@sjpotter if there is an ack from @phoracek can you please squash the commits ?

sjpotter · 2019-05-01T14:39:41Z

do you guys want a form of history preserved in the squash or just what's basically the first message.

phoracek · 2019-05-01T15:03:43Z

@sjpotter just the first one

sjpotter · 2019-05-02T21:29:06Z

can't seem to figure out why github still has "changes requested".

SchSeba · 2019-05-03T09:00:13Z

@sjpotter sorry but we merge a PR can you please rebase on master and fix the conflict

SchSeba · 2019-05-06T10:34:00Z

@sjpotter I still see a conflict problem can you please fix it?

sjpotter · 2019-05-06T12:19:05Z

I've been away this weekend and today can get to it tomorrow.

…

On Mon, May 6, 2019, 6:34 AM Sebastian Scheinkman ***@***.***> wrote: @sjpotter <https://github.com/sjpotter> I still see a conflict problem can you please fix it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#89 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMVGV3UMJFQK2INHGSIP73PUACRTANCNFSM4HIGA7MQ> .

integrate https://github.com/nmstate/kubernetes-nmstate into the operator

sjpotter · 2019-05-09T15:45:06Z

the above / continued rebase issue is due to whitespace, so every time a new version of an image is comitted, I have to rebase, as my PR changes whitespace in all the images. I could insert a blank line in the consts, to avoid gofmt adding whitespace.

phoracek · 2019-05-09T15:51:00Z

@sjpotter sorry about that. Let's wait for CI to pass and then I will merge.

phoracek · 2019-05-09T19:47:14Z

ci test please

SchSeba · 2019-05-09T20:27:53Z

pkg/network/nmstate.go

+
+	// render the manifests on disk
+	data := render.MakeRenderData()
+	data.Data["NMStateStateHandlerImage"] = os.Getenv("NMSTATE_STATE_HANDLER_IMAGE")


related to CI error

status: conditions: - lastProbeTime: 2019-05-09T20:06:57Z lastTransitionTime: 2019-05-09T20:06:10Z message: 'failed to render: failed to render nmstate state handler manifests: error rendering manifests: failed to render manifest data/nmstate/002-nmstated.yaml: template: data/nmstate/002-nmstated.yaml:22:20: executing "data/nmstate/002-nmstated.yaml" at <.NMStateHandlerImage>: map has no entry for key "NMStateHandlerImage"' reason: FailedToRender status: "True" type: Failing FAILED

can you please change here to

NMStateStateHandlerImage -> NMStateHandlerImage

I think we said we wanted it to be verbosely named? unless the same image will be used for both controller and client?

I've changed the template to be using the correct variable as defined in pkg/network/nmstate.go

SchSeba · 2019-05-11T08:33:30Z

ci tests please

SchSeba · 2019-05-11T08:38:26Z

ci test please

SchSeba

LGTM!

@sjpotter thanks for the contribution great job!

For reference:
https://kubevirt-master-stdci-production.apps.ovirt.org/job/kubevirt_cluster-network-addons-operator_standard-check-pr/98/artifact/ci_build_summary.html

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Apr 24, 2019

SchSeba suggested changes Apr 24, 2019

View reviewed changes

phoracek requested changes Apr 29, 2019

View reviewed changes

data/nmstate/002-nmstated.yaml Outdated Show resolved Hide resolved

pkg/network/nmstate.go Outdated Show resolved Hide resolved

SchSeba suggested changes Apr 29, 2019

View reviewed changes

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 29, 2019

SchSeba reviewed Apr 30, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

phoracek requested changes Apr 30, 2019

View reviewed changes

cluster/functest.sh Outdated Show resolved Hide resolved

pkg/apis/networkaddonsoperator/v1alpha1/networkaddonsconfig_types.go Outdated Show resolved Hide resolved

sjpotter mentioned this pull request May 1, 2019

Add support to operator to manage nmstate state on each node #91

Closed

SchSeba reviewed May 1, 2019

View reviewed changes

phoracek previously approved these changes May 1, 2019

View reviewed changes

sjpotter dismissed phoracek’s stale review via da0976e May 1, 2019 15:18

sjpotter changed the title ~~WIP: add nmstate handler daemonset to operator~~ Add nmstate handler daemonset to operator May 1, 2019

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 1, 2019

kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 3, 2019

Add nmstate state/controller handler daemonset to operator

5abca84

integrate https://github.com/nmstate/kubernetes-nmstate into the operator

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2019

phoracek previously approved these changes May 9, 2019

View reviewed changes

phoracek requested a review from SchSeba May 9, 2019 15:52

SchSeba approved these changes May 9, 2019

View reviewed changes

SchSeba suggested changes May 9, 2019

View reviewed changes

fix template bug due to variable renaming

73f8a1d

sjpotter dismissed phoracek’s stale review via 73f8a1d May 11, 2019 01:50

SchSeba approved these changes May 13, 2019

View reviewed changes

SchSeba merged commit 0074444 into kubevirt:master May 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nmstate handler daemonset to operator #89

Add nmstate handler daemonset to operator #89

sjpotter commented Apr 24, 2019

ovirt-infra commented Apr 24, 2019

SchSeba left a comment

phoracek left a comment

SchSeba left a comment

SchSeba left a comment

SchSeba left a comment

sjpotter commented May 1, 2019

phoracek commented May 1, 2019

sjpotter commented May 2, 2019

SchSeba commented May 3, 2019

SchSeba commented May 6, 2019

sjpotter commented May 6, 2019 via email

sjpotter commented May 9, 2019

phoracek commented May 9, 2019

phoracek commented May 9, 2019

SchSeba May 9, 2019

sjpotter May 11, 2019

sjpotter May 11, 2019

SchSeba commented May 11, 2019

SchSeba commented May 11, 2019

SchSeba left a comment

Add nmstate handler daemonset to operator #89

Add nmstate handler daemonset to operator #89

Conversation

sjpotter commented Apr 24, 2019

ovirt-infra commented Apr 24, 2019

SchSeba left a comment

Choose a reason for hiding this comment

phoracek left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

sjpotter commented May 1, 2019

phoracek commented May 1, 2019

sjpotter commented May 2, 2019

SchSeba commented May 3, 2019

SchSeba commented May 6, 2019

sjpotter commented May 6, 2019 via email

sjpotter commented May 9, 2019

phoracek commented May 9, 2019

phoracek commented May 9, 2019

SchSeba May 9, 2019

Choose a reason for hiding this comment

sjpotter May 11, 2019

Choose a reason for hiding this comment

sjpotter May 11, 2019

Choose a reason for hiding this comment

SchSeba commented May 11, 2019

SchSeba commented May 11, 2019

SchSeba left a comment

Choose a reason for hiding this comment