Implement persistent state #77

tyll · 2018-07-31T16:11:48Z

This is still WIP, it still needs docs and example updates. Just want to use the CI to test the code.

coveralls · 2018-07-31T16:15:06Z

Pull Request Test Coverage Report for Build 172

0 of 0 changed or added relevant lines in 0 files are covered.
802 unchanged lines in 1 file lost coverage.
Overall coverage increased (+1.07%) to 38.698%

Files with Coverage Reduction	New Missed Lines	%
/home/travis/build/linux-system-roles/network/library/network_connections.py	802	19.04%

Totals
Change from base Build 151:	1.07%
Covered Lines:	779
Relevant Lines:	2013

💛 - Coveralls

tyll · 2018-08-13T17:28:38Z

@pcahyna I updated the README and tests, can you please review?

pcahyna · 2018-08-14T14:55:49Z

README.md

+It defines whether the profile is activated (`up`) or deactivated (`down`). If
+it is unset, the profile's runtime state will not be changed.
+
+The `interface_state` setting is either `present` (default) or `absent`. If the


ITYM persistent_state, not interface_state.

pcahyna · 2018-08-14T14:57:11Z

README.md

+
+The `interface_state` setting is either `present` (default) or `absent`. If the
+connection profile contains a `type` setting, the profile will be created or
+updated if the `interface_state` is `present`. If the profile is incomplete,


ditto. (also should probably read "if persistent_state is present" or "if the persistent_state setting is present".)

pcahyna · 2018-08-14T14:58:52Z

README.md

-with the given configuration. It has implicitly `state` `present`, due to the
-presence of `type`. On the other hand, the `present` state requires at least a `type`
-variable.
+with the given configuration. It has implicitly `persisntent_state` `present`.


pcahyna · 2018-08-14T15:00:43Z

README.md

+Connection will complete in the background, for example after a DHCP lease was
+received. `wait: <SECONDS>` is a timeout for how long we give the device to
+activate. The default is `wait=-1` which uses a suitable timeout. Note that
+this argument only makes sense for NetworkManager.
 **TODO** `wait` different from zero is not yet implemented.


wait different from zero is not yet implemented. - but the default is -1, is this not implemented?

From what I understand, it does not seem to be possibly to specify wait:-1. Since there is no change for the behavior in the code and I just fixed the wording here we should probably investigate this later.

I filed #79 for this.

it does not seem to be possibly to specify wait:-1

Can we then write "The default is to use a suitable timeout." without referring to the special -1 value?

pcahyna · 2018-08-14T15:03:24Z

README.md

@@ -358,7 +363,7 @@ Device types like `bridge`, `bond`, `team` work similar:
 network_connections:
  - name: "br0"


If quotes were removed around eth0 above, they should be here as well for consistency.

I removed the quotes from other appearances as well.

pcahyna · 2018-08-14T15:20:56Z

tests/tests_unit.yml

@@ -3,27 +3,32 @@
 - hosts: all
  name: Setup for test running
  tasks:
-    # FIXME: change with_items to loop when test-harness is updated
+    - name: Install EPEL if necessary


I would say "if necessary to get python2-mock", because that's the intent, right?

yes, changed

pcahyna · 2018-08-14T16:58:37Z

module_utils/network_lsr/argument_validator.py

@@ -612,7 +611,8 @@ def _validate_post(self, value, name, result):

 class ArgValidator_DictConnection(ArgValidatorDict):

-    VALID_STATES = ["up", "down", "present", "absent", "wait"]
+    VALID_STATES = ["up", "down"]
+    VALID_PERSISTENT_STATES = ["absent", "present"]


Is it correct that "wait" has disappeared?

Yes, it was not documented before and it just resulted in time.sleep().

pcahyna · 2018-08-14T17:19:54Z

README.md

-`persistent_state` `present` does not directly result in a change in the
-network configuration. That is, the profile is only created or modified, not
-activated.
+The `persistent_state` setting `present` does not directly result in a change


I would write The persistent_state value of present

Changed it to

The value `present` for the `persistent_state` setting

pcahyna · 2018-08-14T17:21:16Z

library/network_connections.py

-            prefix = "#%s, state:%s" % (idx, c["state"])
-            if c["state"] != "wait":
-                prefix = prefix + (", '%s'" % (c["name"]))
+            prefix = "#%s, state:%s persistent_satte:%s" % (


pcahyna · 2018-08-14T17:41:52Z

README.md

@@ -88,7 +88,6 @@ incomplete (lacks the `type` setting) and `persistent_state` is `present`,
 the behavior is undefined. The value `absent` makes the role ensure
 that the profile is not present on the target host.

-


Sorry that was a mistake. The commit should be dropped.

ok, removed it

Splitting the role in smaller parts helps to keep the overview and to develop separate tests.

This allows to up profiles that are only available on disk.

This keeps the output more readable.

- persistent_state represents whether a profile is stored on disk - persistent_state defaults to 'present' - When there is no type specified for the profile, it is enough for a profile with the same name to be stored on the target's systems file system. Otherwise the role will fail - state now represents the runtime state and can be up, down or unspecified - translate the state definitions into actions that will be performed. The actions correspond to the previous states. - add the possibility to write unit tests to only verify parts of the resulting connection dictionary to only check for the expected changes instead of the full connection that can also contain unrelated defaults

tyll force-pushed the persistent_state branch 8 times, most recently from 6ab9ea2 to 1be058b Compare August 2, 2018 12:19

tyll force-pushed the persistent_state branch 2 times, most recently from 3e986b4 to 0b7c0c0 Compare August 13, 2018 17:27

tyll requested review from pcahyna and thom311 August 13, 2018 17:28

pcahyna reviewed Aug 14, 2018

View reviewed changes

tyll force-pushed the persistent_state branch from 46ef16b to 8c544df Compare August 14, 2018 17:14

pcahyna reviewed Aug 14, 2018

View reviewed changes

tyll force-pushed the persistent_state branch 4 times, most recently from 1af1b3b to 51abe68 Compare August 14, 2018 18:36

Modularize role

382c341

Splitting the role in smaller parts helps to keep the overview and to develop separate tests.

tyll added 7 commits August 15, 2018 10:04

Do not require profiles to be specified for 'up'

d76ac65

This allows to up profiles that are only available on disk.

Do not log long list of installed packages

987c8d0

This keeps the output more readable.

Use loop to install packages

e1bb399

Support compatibility values for 'state' setting

09258d8

Do not mention wait:-1

53ee0e3

Fix typo

712ff3f

tyll force-pushed the persistent_state branch from 8ab5b9b to 712ff3f Compare August 15, 2018 08:05

This was referenced Aug 15, 2018

warning in docs #80

Merged

Inconsistent behavior for bridges on RHEL6 and RHEL7 #53

Closed

pcahyna merged commit 712ff3f into linux-system-roles:master Aug 21, 2018

tyll deleted the persistent_state branch January 11, 2019 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement persistent state #77

Implement persistent state #77

tyll commented Jul 31, 2018

coveralls commented Jul 31, 2018 •

edited

tyll commented Aug 13, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

pcahyna Aug 14, 2018

tyll Aug 14, 2018

		@@ -358,7 +363,7 @@ Device types like `bridge`, `bond`, `team` work similar:
		network_connections:
		- name: "br0"

		@@ -88,7 +88,6 @@ incomplete (lacks the `type` setting) and `persistent_state` is `present`,
		the behavior is undefined. The value `absent` makes the role ensure
		that the profile is not present on the target host.

Implement persistent state #77

Implement persistent state #77

Conversation

tyll commented Jul 31, 2018

coveralls commented Jul 31, 2018 • edited

Pull Request Test Coverage Report for Build 172

💛 - Coveralls

tyll commented Aug 13, 2018

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

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

coveralls commented Jul 31, 2018 •

edited