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 one-off command #65

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

peterfication
Copy link

This command takes the image of the container of the current pod
and runs the specified command in a one-off kubectl run command.

If the pod has multiple containers and none is specified, it will
fail asking for the specific container.

This command takes the image of the container of the current pod
and runs the specified command in a one-off kubectl run command.

If the pod has multiple containers and none is specified, it will
fail asking for the specific container.
@peterfication
Copy link
Author

peterfication commented Jun 16, 2018

I might have done some things wrong, so I'm really happy to receive feedback on this!

src/cmd.rs Outdated
.arg("--context")
.arg(&kluster.name)
.arg("run")
.arg("one-off-shell")
Copy link
Author

Choose a reason for hiding this comment

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

Could be named more specific eg. <pod_name>-one-off

src/cmd.rs Outdated
@@ -1145,6 +1145,199 @@ command!(
}
);

command!(
OneOff,
Copy link
Author

Choose a reason for hiding this comment

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

OneOff should be probably Run to mimic the kubectl command.

@peterfication
Copy link
Author

Implements #64
/cc @nicklan

@peterfication
Copy link
Author

Would be useful to have the env variables of the specified pod set as well.

@peterfication
Copy link
Author

The challenge with the env variables is, that there might be some variables from secrets and kubectl run does not support this, see kubernetes/kubernetes#48684. But there is a work around described here: kubernetes/kubernetes#48684 (comment)

@nicklan
Copy link
Collaborator

nicklan commented Aug 10, 2018

This looks great! I'm just gonna test it out and then will leave comments and/or merge

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

looks pretty good, modulo needing to fix the name

src/cmd.rs Outdated
clickwrite!(writer, "Couldon't get container image\n");
return;
};
let pod_name = format!("one-off-shell-{}", container_image.as_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to truncate to 63 characters, and also needs to replace any invalid or uppercase characters. from k8s: must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

From k8s:
```
must consist of lower case alphanumeric characters, '-' or '.', and must start
and end with an alphanumeric character (e.g. 'example.com', regex used for
validation is
'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'
```
@peterfication
Copy link
Author

@nicklan thanks for pointing that out. I added a method to sanitise the pod name.

@peterfication
Copy link
Author

I realised that the current implementation only allows one run command of the same image at the same time. A random sequence could be added to the K8s name so it does not fail for more run commands for the same pod/image at the same time.

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

Thanks for the fixup, it works on some stuff now, but short pod names cause a panic.

let pod_name_regex = Regex::new(r"[^a-z0-9-]").unwrap();
let result = pod_name_regex.replace_all(&s.to_ascii_lowercase().to_string(), "-").to_string();

if &result[0..1] == "-" || &result[58..59] == "-" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't hard-code up to 59 here since the pod name might be less than 59 characters which will cause a panic. You'll need to look at the string length and go off that.

Copy link
Author

Choose a reason for hiding this comment

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

Oh no that's not good. I also realized that you can't run a command for the same pod twice. So while fixing this I would also introduce a random string to accommodate that use case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's true. The other option is that you can delete the pod when it's done. Maybe the right thing to do would be to default to adding a random string, but have an option like --name that lets you set the name you want to use

Copy link
Author

Choose a reason for hiding this comment

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

Good point!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants