tuning: revert values on delete #540

ipatrykx · 2020-10-09T10:28:16Z

This PR will introduce NIC's attributes restoration as described in Issue #493: Tuning Plugin: Revert Values on Delete.

Currently when NIC's attributes (MAC, promiscuous mode, MTU) are changed by Tuning plugin, they remain as set even when pod is deleted and NIC is moved back to host's networking namespace. With this PR, JSON-based backup file will be created in dataDir (defaults to: /var/lib/cni/tuning), and it will be used to restore the NIC's attributes to original ones on pod deletion.

Readme and tests were updated as well to reflect those changes.

Values changed by Tuning plugin should be changed only for pod, therefore should be reverted when NIC is being moved from pod back to host. Fixes issue containernetworking#493 Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed · 2020-10-14T16:05:15Z

Any reason not to restore sysctls as well?

bboreham

Tens of lines of cosmetic changes make a PR harder to review, so please take them out.
If you like, make a separate PR that is just those changes.

bboreham · 2020-10-14T15:56:27Z

plugins/meta/tuning/tuning.go

@@ -258,7 +405,7 @@ func cmdCheck(args *skel.CmdArgs) error {

 	// Parse previous result.
 	if tuningConf.RawPrevResult == nil {
-		return fmt.Errorf("Required prevResult missing")
+		return fmt.Errorf("required prevResult missing")


I believe I have deleted all the cosmetic changes.

plugins/meta/tuning/tuning_test.go

mars1024 · 2020-10-15T09:10:43Z

plugins/meta/tuning/tuning.go

+	if err != nil {
+		return fmt.Errorf("failed to marshall data for %q: %v", ifName, err)
+	}
+	if err = ioutil.WriteFile(path.Join(backupDir, ifName+".json"), data, 0600); err != nil {


Is it a wise choice to cache something inside container? Or /var/lib/cni/ on host is a better place? which is just being used by IPAM plugin host-local.

Backup file is stored in host's filesystem (/var/lib/cni/tunig by default). I think that storing backup in the container wouldn't be wise as container is not exactly persistent and might be for example deleted outside of the K8s. Additionally I think that if NIC is host's resource, therefore NIC's config should be stored on host itself.

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

ipatrykx · 2020-10-20T07:55:01Z

Any reason not to restore sysctls as well?

@squeed I believe that sysctls are set per namespace so setting those in container does not affect the host's setup, therefore I think that it is not necessary to restore this settings in any way.

user@control-plane ~ $ kubectl exec -it tuning-pod -- cat /proc/sys/net/core/somaxconn
500
user@worker ~ $ cat /proc/sys/net/core/somaxconn
128

squeed · 2020-10-21T16:04:55Z

plugins/meta/tuning/tuning.go

+		}
+	}
+
+	backupDir := path.Join(backupPath, containerID)


Rather than creating a directory per container, just join container id + ifname - that's what we do in other plugins.

Oh, I forgot about tests. Fixed.

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

bboreham

Some thoughts below

bboreham · 2020-10-28T15:46:04Z

plugins/meta/tuning/tuning.go

@@ -205,6 +332,11 @@ func cmdAdd(args *skel.CmdArgs) error {
 			}
 		}

+		isTuned := (tuningConf.Mac != "" || tuningConf.Mtu != 0 || tuningConf.Promisc)
+		if err = createBackup(args.IfName, args.ContainerID, tuningConf.DataDir, isTuned, tuningConf); err != nil {


could we just skip creating a backup if nothing was tuned?

I initially though that it would be better to create a file even if nothing's changed just to avoid using the file existence check as indicator of whenever something has to be restored. However changed that as it seems that it may be more clear in general.

bboreham · 2020-10-28T15:47:32Z

plugins/meta/tuning/tuning.go

@@ -60,6 +66,14 @@ type IPAMArgs struct {
 	Mtu     *int               `json:"mtu,omitempty"`
 }

+// configToRestore will contain interface attributes that should be restored on cmdDel
+type configToRestore struct {


It strikes me that this struct could be a part of TuningConf, which would remove the need to repeat all fields.

(and maybe part of IPAMArgs ?)

@bboreham I've changed the code. Renamed configToRestore to ifConfig and made it a part of IPAMArgs. I chose IPAMArgs because Promisc field in that structure is a pointer to bool same as in configToRestore. In case of json, simple bool will be omitted if it's value will be false and omitempty were specified. And I believe that in case of future development or just to avoid ambiguity it would be better to save exact value of Promisc in backup file whether it is true or false.

Which leads me to another question I was wondering regarding the Promisc bool in TuningConf. In original code of cmdAdd there is

if tuningConf.Promisc != false { if err = changePromisc(args.IfName, true); err != nil { return err } }

So if user will have a NIC with promiscuous mode enabled, and would like to switch promisc off using tuning (by setting promisc to false), then tuning plugin will not disable the promiscuous mode on that NIC as the code inside if statement will not be executed. Is this an expected behaviour, e.g. linked to problems with some particular NIC or driver? If not, then Promisc bool could be changed to pointer, alongside MAC and MTU variables, and therefore ifConfig could be used in both IPAMArgs and TuningConf. I think that this shouldn't be a part of this PR anyway, so this question is just out of curiosity.

bboreham · 2020-10-28T15:49:53Z

plugins/meta/tuning/README.md

@@ -46,6 +46,8 @@ The parameters, "mac", "mtu" and "promisc", changes the interface attributes as
 }
 ```

+All the interface attributes set by tuning plugin will be restored to original values on container's removal.


except sysctls?

I've made the info more precise.

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed · 2020-11-18T16:53:44Z

plugins/meta/tuning/tuning.go

-	Promisc *bool              `json:"promisc,omitempty"`
-	Mtu     *int               `json:"mtu,omitempty"`
+	SysCtl   *map[string]string `json:"sysctl"`
+	IfConfig *ifConfig          `json:"ifConfig,omitempty"`


This would be changing the configuration format, which we shouldn't do. Do you just want to embed the IfConfig struct?

This was changed because of @bboreham request: #540 (comment)
I have explained this under that comment. If you need any more explanation, don't hesitate to ask.

Should I revert this change?

The explanation helps, but it does not override the point that we can't just make a breaking change to the API because you think it's nicer.
I did say "maybe"; if it can't be done, so be it.

@squeed @bboreham Just reverted the API changes.

squeed · 2020-11-25T16:15:19Z

plugins/meta/tuning/tuning.go

+		return fmt.Errorf("failed to remove file %v: %v", filePath, err)
+	}
+
+	if err = delDirIfEmpty(backupPath); err != nil {


Now that we are using a single data directory, there's probably no point in removing the directory at all.

Removed delDirIfEmpty and it's invocation.

squeed · 2020-11-25T16:17:13Z

plugins/meta/tuning/tuning.go

@@ -37,9 +40,12 @@ import (
 	bv "github.com/containernetworking/plugins/pkg/utils/buildversion"
 )

+const defaultDataDir = "/var/lib/cni/tuning"


I know we use /var/lib/cni for lots of other plugins, but that was a mistake I'm trying to undo. Can you make the default directory /run/cni/tuning please?

By using /run, we don't have to worry about cleaning up stale containers on reboot.

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed · 2020-12-02T16:14:07Z

plugins/meta/tuning/tuning.go

+
+	config := configToRestore{}
+	if err = json.Unmarshal([]byte(file), &config); err != nil {
+		return fmt.Errorf("failed to unmarshall file %q: %v", filePath, err)


If this fails, just log an error and return; we don't want to block deletion.

How should be an error logged?
Returned nil for now.

squeed · 2020-12-02T16:19:26Z

plugins/meta/tuning/tuning.go

+
+	_, err = netlink.LinkByName(ifName)
+	if err != nil {
+		return fmt.Errorf("failed to get %q: %v", ifName, err)


Log here and return; the interface is clearly deleted :-)

Same as above - just returned nil for now.

squeed · 2020-12-02T16:19:39Z

plugins/meta/tuning/tuning.go

+
+	if config.Mtu != 0 {
+		if err = changeMtu(ifName, config.Mtu); err != nil {
+			err = fmt.Errorf("failed to restore MAC address: %v", err)


"failed to restore MTU"

squeed · 2020-12-02T16:28:36Z

plugins/meta/tuning/tuning.go

+		return err
+	}
+
+	err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {


now that I think about it, do we want to block deletion at all if this fails? When would there be an error that could be recoverable on retry?

OK, I think you're right. Changed this to return always nil at the end, same as the 'stub' function that was here previously.

Should this

tuningConf, err := parseConf(args.StdinData, args.Args) if err != nil { return err }

also be forced to return nil?

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed · 2020-12-09T16:45:04Z

looks good to me.

ipatrykx force-pushed the tuning-revert branch from c837d7a to 8e842c4 Compare October 9, 2020 10:48

bboreham reviewed Oct 14, 2020

View reviewed changes

mars1024 reviewed Oct 15, 2020

View reviewed changes

Reverted cosmetic changes

2ac93c3

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

ipatrykx force-pushed the tuning-revert branch from 61b8acb to 2ac93c3 Compare October 15, 2020 10:36

ipatrykx requested review from bboreham and mars1024 October 15, 2020 10:45

squeed reviewed Oct 21, 2020

View reviewed changes

ipatrykx added 3 commits October 22, 2020 15:55

tuning: changed backup path

e34baa4

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

tuning: updated test with new backup filename

d94c447

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

tuning: fixed gofmt errors

7e2c95b

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

bboreham reviewed Oct 28, 2020

View reviewed changes

tuning: Backup created only if necessary. Minor fix in README.

16f2dd3

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed reviewed Nov 18, 2020

View reviewed changes

ipatrykx force-pushed the tuning-revert branch from 5739f37 to 16f2dd3 Compare November 25, 2020 11:24

Fixed README.md conflicts

0c49a97

squeed reviewed Nov 25, 2020

View reviewed changes

Changed default backup directory and removed backup directory deletion.

861e91a

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed reviewed Dec 2, 2020

View reviewed changes

Error handling fixed

beff202

Signed-off-by: Patryk Strusiewicz-Surmacki <patrykx.strusiewicz-surmacki@intel.com>

squeed merged commit e13bab9 into containernetworking:master Dec 9, 2020

mikebrow mentioned this pull request Jan 26, 2021

[WIP] Update to latest sha/tag of dependencies containerd/containerd#4970

Closed

tuning: revert values on delete #540

tuning: revert values on delete #540

Conversation

ipatrykx commented Oct 9, 2020

squeed commented Oct 14, 2020

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipatrykx commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipatrykx Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squeed commented Dec 9, 2020

ipatrykx Dec 4, 2020 •

edited

Loading