Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ank apply command to CLI #180

Merged
merged 28 commits into from
Feb 28, 2024
Merged

Add ank apply command to CLI #180

merged 28 commits into from
Feb 28, 2024

Conversation

lingnoi
Copy link
Contributor

@lingnoi lingnoi commented Feb 5, 2024

Issues: #26

Added ank apply command

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.

@lingnoi lingnoi mentioned this pull request Feb 5, 2024
1 task
@lingnoi lingnoi requested a review from krucod3 February 5, 2024 07:45
@windsource

This comment was marked as resolved.

@windsource

This comment was marked as resolved.

@windsource

This comment was marked as resolved.

@lingnoi lingnoi added the blocked label Feb 6, 2024
@inf17101
Copy link
Contributor

inf17101 commented Feb 8, 2024

Maybe one suggestion and meant optionally (It expects how much workloads we expect on average touched with this new command): The ank apply command outputs the workloads it has touched to the console within { ... } which is nice. However, if there are many workloads maybe the output is too much in the console and not helpful anymore. Maybe we can limit the output to a certain number and output then { w1, w2, w3, ... } or showing only in verbose mode all of them if someone needs that really for debugging.

@windsource
Copy link
Contributor

windsource commented Feb 8, 2024

Maybe one suggestion and meant optionally (It expects how much workloads we expect on average touched with this new command): The ank apply command outputs the workloads it has touched to the console within { ... } which is nice. However, if there are many workloads maybe the output is too much in the console and not helpful anymore. Maybe we can limit the output to a certain number and output then { w1, w2, w3, ... } or showing only in verbose mode all of them if someone needs that really for debugging.

@inf17101 Not sure what you mean. When I call ank apply it only returns the names of the affected agents and not of the workloads.

❯ ank apply databroker.yaml
agent_names={"agent_A"}
error: ExecutionError("Failed get response from server in time (timeout=3s).")

However, I do not think that we shoud limit those.

@inf17101
Copy link
Contributor

inf17101 commented Feb 8, 2024

Maybe one suggestion and meant optionally (It expects how much workloads we expect on average touched with this new command): The ank apply command outputs the workloads it has touched to the console within { ... } which is nice. However, if there are many workloads maybe the output is too much in the console and not helpful anymore. Maybe we can limit the output to a certain number and output then { w1, w2, w3, ... } or showing only in verbose mode all of them if someone needs that really for debugging.

@inf17101 Not sure what you mean. When I call ank apply it only returns the names of the affected agents and not of the workloads.

❯ ank apply databroker.yaml
agent_names={"agent_A"}
error: ExecutionError("Failed get response from server in time (timeout=3s).")

However, I do not think that we shoud limit those.

If you would not have the response errors (because of missing @christoph-hamm' PR) then it would print the workloads as well and potentially spam the output if there are many to touch. But it is anyway meant optionally. But I thought it is maybe helpful to mention, because today's applications would handle such case. I think it is not a bad idea to restrict output that can be not calculated how much it would be.

do folding of workload names when versobse mode is off
@windsource

This comment was marked as resolved.

Copy link
Contributor

@windsource windsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting multiple agents in yaml files provided to ank apply is required.

@windsource

This comment was marked as resolved.

@lingnoi
Copy link
Contributor Author

lingnoi commented Feb 9, 2024

It is not possible to deploy workloads on different agents.

❯ ank apply *.yaml
error: Multiple agent names in manifests detected ["agent_A", "infotainment"] -> use '--agent' option to overwrite! 

I do not understand that message. Why should it be forbidden to deploy workloads on different agents and how does the option --agent help here?

Currently it is not supported and you need to tell via the --agent option on which agent to apply the workloads. If it is required then I will adapt the code to support multiple agents provided in the workload specs. Is it required?

@windsource
Copy link
Contributor

It is not possible to deploy workloads on different agents.

❯ ank apply *.yaml
error: Multiple agent names in manifests detected ["agent_A", "infotainment"] -> use '--agent' option to overwrite! 

I do not understand that message. Why should it be forbidden to deploy workloads on different agents and how does the option --agent help here?

Currently it is not supported and you need to tell via the --agent option on which agent to apply the workloads. If it is required then I will adapt the code to support multiple agents provided in the workload specs. Is it required?

When an application developer provides an application with workloads for different agents then one manifest should be enough to describe the deployment of that application. So I think it required.

ank/src/cli.rs Outdated Show resolved Hide resolved
@windsource
Copy link
Contributor

Currently the output does not look very clear. What about using a table structure as output?

$ ank apply file1.yaml file2.yaml
WORKLOAD    AGENT    STATUS  FILENAME    
databroker  agent_a  OK      file1.yaml  
nginx       agent_b  Error   file1.yaml  
db          agent_b  OK      file2.yaml  

Co-authored-by: Holger Dormann <holger.dormann@elektrobit.com>
@lingnoi
Copy link
Contributor Author

lingnoi commented Feb 12, 2024

Currently the output does not look very clear. What about using a table structure as output?

$ ank apply file1.yaml file2.yaml
WORKLOAD    AGENT    STATUS  FILENAME    
databroker  agent_a  OK      file1.yaml  
nginx       agent_b  Error   file1.yaml  
db          agent_b  OK      file2.yaml  

Okay, I like your suggestion! I understood that the current output contains some kind of redundant information. I will adapt it.

@christoph-hamm christoph-hamm removed their assignment Feb 23, 2024
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/src/cli_commands.rs Outdated Show resolved Hide resolved
common/src/state_manipulation/object.rs Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
@maturar
Copy link
Contributor

maturar commented Feb 23, 2024

I found a bug when I have tried to overwrite the agent with the --agent flag.
I have created workloads with the command ./ank apply --agent agent_B manifest.yaml. It means that the flag --agent agent_B ovewrites the configuration in the manifest.yaml (here is the agent set to agent_A. The command produces following ouptut:

 WORKLOAD NAME   AGENT     OPERATION    STATUS   FILE          
 hello1          agent_A   Add/Update   OK       manifest.yaml 
 nginx           agent_A   Add/Update   OK       manifest.yaml 

But in the reality the Ankaios creates workloads in agent_B:

$ ./ank get workloads
 WORKLOAD NAME   AGENT     RUNTIME   EXECUTION STATE   ADDITIONAL INFO 
 hello1          agent_B   podman    Failed(Lost)                      
 nginx           agent_B   podman    Running(Ok)                       

The information printed by the ank apply is not consistent with the real state.

The output of ank apply .. is not the same as ank get workloads. As it is a diffrent command. The "STATUS" of ank apply .. shall not represent the "EXECUTION STATE", instead it represent the apply status. Why should this status be the same as execution state?

Well for me as a user it is confusing if I say that I would like to overwrite the agent name with the flag and the command answers with something else. My understanding is that the table shall represent a summary what information the command ank apply is going to use.
BTW: I was reading all comments here and I found out this comment: #180 (comment) Is not this the same issue?

If the agent name is meant then it is the same issue!

The issue has been fixed.

ank/src/cli_commands.rs Show resolved Hide resolved
ank/doc/swdesign/README.md Show resolved Hide resolved
ank/src/cli_commands.rs Outdated Show resolved Hide resolved
ank/src/cli_commands.rs Outdated Show resolved Hide resolved
tests/resources/configs/manifest1.yaml Outdated Show resolved Hide resolved
@windsource
Copy link
Contributor

windsource commented Feb 27, 2024

I have created two workloads with:

❯ ank apply --agent infotainment speed-consumer.yaml speed-provider.yaml
 WORKLOAD NAME    AGENT          OPERATION    STATUS   FILE
 speed-consumer   infotainment   Add/Update   OK       speed-consumer.yaml
 speed-provider   infotainment   Add/Update   OK       speed-provider.yaml

and the output looks OK. But when I delete those workloads with:

❯ ank apply -d speed-consumer.yaml speed-provider.yaml
 WORKLOAD NAME    AGENT     OPERATION   STATUS   FILE
 speed-consumer             Remove      OK       speed-consumer.yaml
 speed-provider   agent_A   Remove      OK       speed-provider.yaml

the agent names are wrong. As the agent names are not known they column AGENT shall be removed for the output of ank apply -d .

lingnoi and others added 2 commits February 27, 2024 14:14
print no agent names on operation remove
ank/doc/swdesign/README.md Show resolved Hide resolved
ank/doc/swdesign/README.md Show resolved Hide resolved
tests/resources/configs/manifest1.yaml Outdated Show resolved Hide resolved
removed unsupported fields in test data
Copy link
Contributor

@windsource windsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK from a user perspective. Still some approval from technical perspective required.

@maturar
Copy link
Contributor

maturar commented Feb 27, 2024

There is a merge conflict in this PR. @lingnoi could you resolve it, please?

Copy link
Contributor

@maturar maturar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay for me. @christoph-hamm is reviewing fixes of his findings in the swdesign.

Copy link
Contributor

@christoph-hamm christoph-hamm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two small findings.

ank/src/cli_commands.rs Outdated Show resolved Hide resolved
ank/doc/swdesign/README.md Outdated Show resolved Hide resolved
lingnoi and others added 3 commits February 28, 2024 15:08
Co-authored-by: Christoph Hamm <130038849+christoph-hamm@users.noreply.github.com>
Co-authored-by: Christoph Hamm <130038849+christoph-hamm@users.noreply.github.com>
removed underscore
@lingnoi lingnoi merged commit d3b2f6a into main Feb 28, 2024
7 checks passed
@lingnoi lingnoi deleted the 26-ank_apply_command branch February 28, 2024 14:56
@windsource windsource added the enhancement New feature or request. Issue will appear in the change log "Features" label Apr 4, 2024
@windsource windsource changed the title 26 ank apply command Add ank apply command to CLI Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Issue will appear in the change log "Features" ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants