Change restart flag to an restart policy enum in interfaces #217

inf17101 · 2024-03-26T12:31:53Z

Issues: #5, #212

Changes:

Replaced the restart bool flag in all public interfaces and internal objects
Fixed the utests using the restart enum
Adapted the user docs refering to the restart configuration item
Explicitly made "restart: NEVER" the default for all existing stest configs to prevent them from different behavior when implementing the actual restart behavior
introduced a new stest config containing a startup state with all the different restart policies
adapted the server/resources/startConfig.yaml file to include the types of the restart policies
added Swdd item explaining the support of the new restart policy values

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

All steps in CONTRIBUTING.md have been handled

inf17101 · 2024-03-26T12:34:35Z

@krucod3: After all local modifications, I had an enum called RestartPolicy. But then, I thought it is too long and the old name is also suitable. So, I have renamed it to Restart again. Please let me know if you prefer RestartPolicy.

inf17101 · 2024-03-26T12:41:33Z

Shall I add an comment out stest also?

This reverts commit d8c2cdd.

krucod3

All in all looks good, just a couple of small findings.

doc/docs/reference/complete-state.md

server/resources/startConfig.yaml

examples/nodejs_control_interface/src/main.js

inf17101 · 2024-03-27T07:45:55Z

@krucod3: Since a restart of a workload is basically an update (delete + create), I would recommend that we rename the Restart stuff related to the retry mechanism inside the WorkloadControlLoop to "Retry" instead of "Restart". This prevents confusion (especially for the swdd chapters).

agent/doc/swdesign/README.md

doc/docs/reference/complete-state.md

examples/config/startupState.yaml

examples/cpp_control_interface/src/main.cpp

examples/nodejs_control_interface/src/main.js

tests/resources/configs/state_with_dependency_cycle.yaml

tests/resources/configs/update_state_config_with_dependency_cycle.yaml

tests/resources/configs/update_state_create_one_workload.yaml

tools/install.sh

Co-authored-by: lingnoi <42992756+lingnoi@users.noreply.github.com>

krucod3 · 2024-03-28T11:55:06Z

We should not just update all configs to ALWAYS if the previous value was true. Workloads that go to succeeded will end up in a restart loop and this is not what we are targeting.

inf17101 · 2024-03-28T12:02:09Z

We should not just update all configs to ALWAYS if the previous value was true. Workloads that go to succeeded will end up in a restart loop and this is not what we are targeting.

Yes this is why I have carefully thought about the need for restart or not for each workload config or documentation snippet. And I have disabled the restart feature for all stests not testing the restart feature itself to prevent stests from failing because of the restart failure even if their intend tests a different feature.

lingnoi · 2024-03-28T12:21:28Z

@krucod3: After all local modifications, I had an enum called RestartPolicy. But then, I thought it is too long and the old name is also suitable. So, I have renamed it to Restart again. Please let me know if you prefer RestartPolicy.

I think, utilizing the term "RestartPolicy" is more precise, as the term "Restart" alone could potentially be misunderstood in various contexts.

inf17101 · 2024-03-28T13:05:44Z

@krucod3: After all local modifications, I had an enum called RestartPolicy. But then, I thought it is too long and the old name is also suitable. So, I have renamed it to Restart again. Please let me know if you prefer RestartPolicy.

I think, utilizing the term "RestartPolicy" is more precise, as the term "Restart" alone could potentially be misunderstood in various contexts.

I will do a commit in a few minutes. But I need to test all examples again after the changes.

inf17101 · 2024-03-28T14:31:54Z

@krucod3: All changes for renaming "restart" into "RestartPolicy" are done (Manifest + Enum). The problem is since two builds in GH actions and on my local side the mkdocs build is failing tools/generate_docs.sh serve with the following error:

It is failing on the main branch as well (see picture above).

The mystique thing is that on @lingnoi WSL2 both main and this branch builds the documentation successfully.

We have found out that it is the social plugin of mkdocs:

If I comment out the social plugin then it works. However, after analyzing the python code of the social plugin it seems like it is trying to download the Roboto.zip of google fonts (this is happen when the documentation is built). The download does not return a valid zip file if you download it in a programmatic way (curl, python code of the function that loads the zip). It returns a javascript file. However, if you download the google fonts with the browser it returns a valid zip file.

The issue was also mentioned in the past here: squidfunk/mkdocs-material#5628 (but with different setting)
So, shall I comment out the social plugin? I think otherwise this PR is blocked from merging. I would open a ticket. I would recommend not relying on external downloads such as google fonts, because it can break something like we see here. Maybe we can host them locally for building if a cooler font is needed or change to a default font maybe.

lingnoi · 2024-03-28T14:35:40Z

@krucod3: All changes for renaming "restart" into "RestartPolicy" are done (Manifest + Enum). The problem is since two builds in GH actions and on my local side the mkdocs build is failing tools/generate_docs.sh serve with the following error:

It is failing on the main branch as well (see picture above).

The mystique thing is that on @lingnoi WSL2 both main and this branch builds the documentation successfully.

We have found out that it is the social plugin of mkdocs:

If I comment out the social plugin then it works. However, after analyzing the python code of the social plugin it seems like it is trying to download the Roboto.zip of google fonts (this is happen when the documentation is built). The download does not return a valid zip file if you download it in a programmatic way (curl, python code of the function that loads the zip). It returns a javascript file. However, if you download the google fonts with the browser it returns a valid zip file.

The issue was also mentioned in the past here: squidfunk/mkdocs-material#5628 (but with different setting) So, shall I comment out the social plugin? I think otherwise this PR is blocked from merging.

I can confirm, I can build and serve the documentation of this branch and also of main branch on my machine (WSL2).

lingnoi · 2024-03-28T14:45:16Z

@krucod3: All changes for renaming "restart" into "RestartPolicy" are done (Manifest + Enum). The problem is since two builds in GH actions and on my local side the mkdocs build is failing tools/generate_docs.sh serve with the following error:

It is failing on the main branch as well (see picture above).
The mystique thing is that on @lingnoi WSL2 both main and this branch builds the documentation successfully.
We have found out that it is the social plugin of mkdocs:
If I comment out the social plugin then it works. However, after analyzing the python code of the social plugin it seems like it is trying to download the Roboto.zip of google fonts (this is happen when the documentation is built). The download does not return a valid zip file if you download it in a programmatic way (curl, python code of the function that loads the zip). It returns a javascript file. However, if you download the google fonts with the browser it returns a valid zip file.
The issue was also mentioned in the past here: squidfunk/mkdocs-material#5628 (but with different setting) So, shall I comment out the social plugin? I think otherwise this PR is blocked from merging.

I can confirm, I can build and serve the documentation of this branch and also of main branch on my machine (WSL2).

After removing the .cache folder, I'm able to reproduce the issue:

lingnoi

LGTM, now 👍

krucod3

LGTM 👍

inf17101 added 2 commits March 26, 2024 13:04

Add restart enum

b56b4f0

Add Swdd item

a4ed283

inf17101 self-assigned this Mar 26, 2024

inf17101 added breaking-change Issue will appear in the change log "Breaking Changes" enhancement New feature or request. Issue will appear in the change log "Features" labels Mar 26, 2024

inf17101 requested review from krucod3 and lingnoi March 26, 2024 12:33

inf17101 added 3 commits March 26, 2024 13:55

Replace fake update with real update

6d9d89f

Satisfy test coverage

d8c2cdd

Revert "Satisfy test coverage"

742b999

This reverts commit d8c2cdd.

krucod3 reviewed Mar 26, 2024

View reviewed changes

doc/docs/reference/complete-state.md Outdated Show resolved Hide resolved

server/resources/startConfig.yaml Show resolved Hide resolved

Fix review findings and make doc equal to startConfig.yaml

cf1c889

maturar reviewed Mar 26, 2024

View reviewed changes

examples/nodejs_control_interface/src/main.js Outdated Show resolved Hide resolved

inf17101 added 3 commits March 26, 2024 16:19

Merge branch 'main' into 212_restart_policy_interface

6f51554

Replace more restart values

cc5d764

Change restart policy in examples

8e92296

inf17101 added 2 commits March 27, 2024 11:24

Rename restart creation of workloads into retry

42c2ff5

Set req id version to 1

40df81f

lingnoi reviewed Mar 28, 2024

View reviewed changes

Update agent/doc/swdesign/README.md

a5eaabf

Co-authored-by: lingnoi <42992756+lingnoi@users.noreply.github.com>

Rename Restart enum and manifest key

de30e78

Disable social mkdocs plugin

4e1a562

lingnoi approved these changes Mar 28, 2024

View reviewed changes

krucod3 approved these changes Apr 2, 2024

View reviewed changes

krucod3 merged commit cfc7e49 into main Apr 2, 2024
7 checks passed

krucod3 deleted the 212_restart_policy_interface branch April 2, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change restart flag to an restart policy enum in interfaces #217

Change restart flag to an restart policy enum in interfaces #217

inf17101 commented Mar 26, 2024 •

edited

inf17101 commented Mar 26, 2024 •

edited

inf17101 commented Mar 26, 2024

krucod3 left a comment

inf17101 commented Mar 27, 2024 •

edited

krucod3 commented Mar 28, 2024

inf17101 commented Mar 28, 2024

lingnoi commented Mar 28, 2024

inf17101 commented Mar 28, 2024

inf17101 commented Mar 28, 2024 •

edited

lingnoi commented Mar 28, 2024

lingnoi commented Mar 28, 2024

lingnoi left a comment

krucod3 left a comment

Change restart flag to an restart policy enum in interfaces #217

Change restart flag to an restart policy enum in interfaces #217

Conversation

inf17101 commented Mar 26, 2024 • edited

Definition of Done

inf17101 commented Mar 26, 2024 • edited

inf17101 commented Mar 26, 2024

krucod3 left a comment

Choose a reason for hiding this comment

inf17101 commented Mar 27, 2024 • edited

krucod3 commented Mar 28, 2024

inf17101 commented Mar 28, 2024

lingnoi commented Mar 28, 2024

inf17101 commented Mar 28, 2024

inf17101 commented Mar 28, 2024 • edited

lingnoi commented Mar 28, 2024

lingnoi commented Mar 28, 2024

lingnoi left a comment

Choose a reason for hiding this comment

krucod3 left a comment

Choose a reason for hiding this comment

inf17101 commented Mar 26, 2024 •

edited

inf17101 commented Mar 26, 2024 •

edited

inf17101 commented Mar 27, 2024 •

edited

inf17101 commented Mar 28, 2024 •

edited