Kill plugin script #12145

danwinship · 2016-12-05T20:21:43Z

The CNI port already got us most of the way there. This moves the remaining bits of pod setup/teardown into go rather than shell script.

(I started this a long time ago, and finished it now because of #11990.)

@openshift/networking PTAL

smarterclayton · 2016-12-05T20:41:54Z

pkg/sdn/plugin/pod_linux.go

+	return otx.EndTransaction()
+}
+
+func (m *podManager) setupPodBandwidth(pod *kapi.Pod, hostVeth string) error {


Are these methods actually related? If all you're accessing is m.ovs taking ovs as an argument is just as good. We generally lean from a style perspective on not creating grouping types unless there is an actual "group".

danwinship · 2016-12-06T13:11:47Z

@smarterclayton like that?

smarterclayton · 2016-12-06T15:07:18Z

pkg/sdn/plugin/pod_linux.go

+	return otx.EndTransaction()
+}
+
+func setupPodBandwidth(ovsif *ovs.Interface, pod *kapi.Pod, hostVeth string) error {


Yeah, looks good. I find a lot of the time it's easy to "over OO" stuff, and I think for many functions you end up with more testable and less coupled code.

dcbw · 2016-12-07T15:12:15Z

@danwinship So generally LGTM, though we'll have the same upgrade problem when a master+node host is updated, since the RPM changes remove the script. Then pods on that node will fail because the script doesn't exist. Maybe we just keep the script around in the RPM (I'm pretty sure we don't need it in the images, just in the specfile) for one more release?

danwinship · 2016-12-07T15:40:59Z

Maybe we just keep the script around in the RPM (I'm pretty sure we don't need it in the images, just in the specfile) for one more release?

openshift doesn't care where the script is as long as it's in $PATH, so we could have the update process copy it from /usr/bin to /usr/local/bin before upgrading the RPM, and then delete it after restarting the node service

danwinship · 2016-12-07T15:41:20Z

(re-push is just to squash the fixup)

smarterclayton · 2016-12-07T16:46:08Z

Generally if it's more complex to do it in ansible, doing it in your code (preserving the script for at least one version) reduces the impact of this change to other teams (keeps your dirty laundry secret)

…

On Wed, Dec 7, 2016 at 10:41 AM, Dan Winship ***@***.***> wrote: (re-push is just to squash the fixup) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12145 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p0HAVONxBM1G6FQSCNoLgZcwZ-ULks5rFtOhgaJpZM4LEqyc> .

knobunc · 2016-12-07T16:47:54Z

Do we only support single version upgrades? I.e. to get from 3.2 to 3.4 you need to go through 3.3 (and start it up)? On Wed, Dec 7, 2016 at 11:46 AM, Clayton Coleman <[email protected]> wrote:

…

Generally if it's more complex to do it in ansible, doing it in your code (preserving the script for at least one version) reduces the impact of this change to other teams (keeps your dirty laundry secret) On Wed, Dec 7, 2016 at 10:41 AM, Dan Winship ***@***.***> wrote: > (re-push is just to squash the fixup) > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#12145 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABG_ p0HAVONxBM1G6FQSCNoLgZcwZ-ULks5rFtOhgaJpZM4LEqyc> > . > — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#12145 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJkavRFjQG2TZAtNHdAHnQ4LkNZbTmvhks5rFuLVgaJpZM4LEqyc> .

danwinship · 2016-12-07T17:22:14Z

(keeps your dirty laundry secret)

It's not clear whose dirty laundry it is; leaving openshift running while changing the rpm out from under it is kind of dirty on their part. But sure, we can leave the script around.

Do we only support single version upgrades?

yes

smarterclayton · 2016-12-09T04:53:19Z

Not quite true. We support single version no downtime upgrades. We do not support multi version no downtime (of the control plane) upgrades, although anything beyond the control plane needs an exception. On Dec 7, 2016, at 12:22 PM, Dan Winship <[email protected]> wrote: (keeps your dirty laundry secret) It's not clear whose dirty laundry it is; leaving openshift running while changing the rpm out from under it is kind of dirty on their part. But sure, we can leave the script around. Do we only support single version upgrades? yes — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12145 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p6ocOKSZ5Vfgq3AcGEiGoxq4dQXeks5rFutHgaJpZM4LEqyc> .

danwinship · 2016-12-17T14:40:04Z

[test][testextended][extended:networking]

danwinship · 2017-01-14T15:43:40Z

Are we going to try to get this in for 3.5? (It's not a feature so I guess it's not subject to the "feature freeze" deadline anyway, but...)

danwinship · 2017-02-21T19:24:00Z

Rebased and repushed now that the tree is open for 3.6. Also changed it to make it continue installing the now-unused script, to avoid problems during upgrade. We can remove it completely for 3.7.

danwinship · 2017-02-22T14:12:45Z

flake #12855, [testextended]

danwinship · 2017-02-25T16:40:49Z

@openshift/networking the test failures will be fixed by #13061 when it lands. This is ready for review.

pravisankar · 2017-03-08T19:03:01Z

pkg/sdn/plugin/pod_linux.go

+	}
+
+	if ovsEgress > 0 {
+		qos, err := ovsif.Create("qos", "type=linux-htb", fmt.Sprintf("other-config:max-rate=%d", ovsEgress))


Where is the code equivalent to "ip link set dev ${veth_host} qlen 1000" in openshift-sdn-ovs?

ah, i think this branch originally predates that addition and I must have missed it when rebasing

pravisankar · 2017-03-08T19:04:47Z

pkg/sdn/plugin/pod_linux.go

+		ovsEgress = podIngress.Value()
+	}
+	if podEgress != nil {
+		ovsIngress = podEgress.Value() / 1024


is it /1024 or /1000? 1024 might be right (we are using 1000 in openshift-sdn-ovs)

Oh, yeah, I saw 1000 in openshift-sdn-ovs but assumed it was probably wrong and should be 1024. @dcbw?

ovs docs say "ingress_policing_rate: ... Maximum rate for data received on this interface, in kbps." so 1024 is correct

pravisankar · 2017-03-08T19:27:12Z

Added couple of comments, rest of the code LGTM.
Need to resolve some merge conflicts

danwinship · 2017-03-08T21:27:13Z

Rebased and added in the qlen thing (and did another check to see if I'd missed anything when rebasing, and didn't see anything). It seems that the netlink library used for all the other /sbin/ip stuff in pod_linux.go doesn't let you do "set qlen" (even if we updated to the latest git) so I had to use ipcmd instead.

openshift-bot · 2017-03-13T13:57:05Z

Evaluated for origin testextended up to b4cac9e

openshift-bot · 2017-03-13T13:57:13Z

Evaluated for origin test up to b4cac9e

openshift-bot · 2017-03-13T15:12:06Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/160/) (Base Commit: 3febe97)

openshift-bot · 2017-03-13T15:45:26Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/19/) (Base Commit: 3febe97) (Extended Tests: networking)

pravisankar

LGTM

danwinship · 2017-03-27T13:57:43Z

[merge]

openshift-bot · 2017-03-27T14:05:23Z

Evaluated for origin merge up to b4cac9e

openshift-bot · 2017-03-27T14:05:32Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/205/) (Base Commit: d260e80) (Image: devenv-rhel7_6104)

Automatic merge from submit-queue Remove no-longer-used openshift-sdn-ovs script This script was not used in 3.6; it was only present to help 3.5->3.6 upgrades work and can now be removed. (#12145 (comment))

danwinship added the component/networking label Dec 5, 2016

smarterclayton reviewed Dec 5, 2016

View reviewed changes

smarterclayton reviewed Dec 6, 2016

View reviewed changes

danwinship force-pushed the kill-plugin-script branch from 7b45817 to 1ac54b6 Compare December 7, 2016 15:38

danwinship mentioned this pull request Dec 15, 2016

new SDN isolation rule organization #11378

Merged

danwinship force-pushed the kill-plugin-script branch from 1ac54b6 to 56853fb Compare December 17, 2016 14:39

danwinship force-pushed the kill-plugin-script branch 2 times, most recently from a566dfa to f1efa1b Compare December 20, 2016 14:38

danwinship force-pushed the kill-plugin-script branch from f1efa1b to d974ae9 Compare January 9, 2017 20:10

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2017

danwinship force-pushed the kill-plugin-script branch from d974ae9 to 7394fb7 Compare January 13, 2017 14:07

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2017

danwinship mentioned this pull request Jan 17, 2017

sdn: add support for multicast traffic for simple and multitenant plugins #12494

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2017

danwinship force-pushed the kill-plugin-script branch 3 times, most recently from ee7ac04 to dd197f1 Compare February 21, 2017 19:23

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2017

danwinship force-pushed the kill-plugin-script branch from dd197f1 to 8f4af41 Compare February 21, 2017 21:57

danwinship force-pushed the kill-plugin-script branch from 8f4af41 to e4c388a Compare March 3, 2017 20:25

danwinship mentioned this pull request Mar 8, 2017

Split out a unit-testable ovsController type in pkg/sdn/plugin #13309

Merged

pravisankar reviewed Mar 8, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2017

danwinship force-pushed the kill-plugin-script branch from e4c388a to 2320693 Compare March 8, 2017 21:26

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2017

danwinship force-pushed the kill-plugin-script branch from 2320693 to 3d8bdc1 Compare March 11, 2017 20:37

danwinship added 2 commits March 13, 2017 09:17

Add ovsdb-manipulating methods to pkg/ovs

b4cac9e

Port openshift-sdn-ovs script to go

972fe78

danwinship force-pushed the kill-plugin-script branch from 3d8bdc1 to 972fe78 Compare March 13, 2017 13:17

pravisankar approved these changes Mar 13, 2017

View reviewed changes

openshift-bot merged commit 8dfe5dd into openshift:master Mar 27, 2017

danwinship deleted the kill-plugin-script branch March 27, 2017 18:27

danwinship mentioned this pull request Aug 8, 2017

Remove no-longer-used openshift-sdn-ovs script #15675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kill plugin script #12145

Kill plugin script #12145

danwinship commented Dec 5, 2016

smarterclayton Dec 5, 2016

danwinship commented Dec 6, 2016

smarterclayton Dec 6, 2016

dcbw commented Dec 7, 2016

danwinship commented Dec 7, 2016

danwinship commented Dec 7, 2016

smarterclayton commented Dec 7, 2016 via email

knobunc commented Dec 7, 2016 via email

danwinship commented Dec 7, 2016

smarterclayton commented Dec 9, 2016 via email

danwinship commented Dec 17, 2016

danwinship commented Jan 14, 2017

danwinship commented Feb 21, 2017

danwinship commented Feb 22, 2017

danwinship commented Feb 25, 2017

pravisankar Mar 8, 2017

danwinship Mar 8, 2017

pravisankar Mar 8, 2017

danwinship Mar 8, 2017

danwinship Mar 27, 2017

pravisankar commented Mar 8, 2017

danwinship commented Mar 8, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

pravisankar left a comment

danwinship commented Mar 27, 2017

openshift-bot commented Mar 27, 2017

openshift-bot commented Mar 27, 2017 •

edited

Loading

Kill plugin script #12145

Kill plugin script #12145

Conversation

danwinship commented Dec 5, 2016

smarterclayton Dec 5, 2016

Choose a reason for hiding this comment

danwinship commented Dec 6, 2016

smarterclayton Dec 6, 2016

Choose a reason for hiding this comment

dcbw commented Dec 7, 2016

danwinship commented Dec 7, 2016

danwinship commented Dec 7, 2016

smarterclayton commented Dec 7, 2016 via email

knobunc commented Dec 7, 2016 via email

danwinship commented Dec 7, 2016

smarterclayton commented Dec 9, 2016 via email

danwinship commented Dec 17, 2016

danwinship commented Jan 14, 2017

danwinship commented Feb 21, 2017

danwinship commented Feb 22, 2017

danwinship commented Feb 25, 2017

pravisankar Mar 8, 2017

Choose a reason for hiding this comment

danwinship Mar 8, 2017

Choose a reason for hiding this comment

pravisankar Mar 8, 2017

Choose a reason for hiding this comment

danwinship Mar 8, 2017

Choose a reason for hiding this comment

danwinship Mar 27, 2017

Choose a reason for hiding this comment

pravisankar commented Mar 8, 2017

danwinship commented Mar 8, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

openshift-bot commented Mar 13, 2017

pravisankar left a comment

Choose a reason for hiding this comment

danwinship commented Mar 27, 2017

openshift-bot commented Mar 27, 2017

openshift-bot commented Mar 27, 2017 • edited Loading

openshift-bot commented Mar 27, 2017 •

edited

Loading