Skip to content

[Openstack Cloud Provider] Added command option to OpenStackInstance to run custom commands#462

Closed
armagankaratosun wants to merge 1 commit intodask:mainfrom
armagankaratosun:main
Closed

[Openstack Cloud Provider] Added command option to OpenStackInstance to run custom commands#462
armagankaratosun wants to merge 1 commit intodask:mainfrom
armagankaratosun:main

Conversation

@armagankaratosun
Copy link
Contributor

Hello all

Adding support to run custom commands on OpenStackInstance Dask Workers. This will be parsed into the cloud-init

  - 'docker run --net=host {%+ if gpu_instance %}--gpus=all{% endif %} {% for key in env_vars %} -e {{key}}="{{env_vars[key]}}" {% endfor %}{%+ if docker_args %}{{docker_args}}{% endif %} {{image}} {{ command }}'

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Given that OpenStackInstance is not intended to be used directly, and passing the command through here is clearly only intended for that use case I'm not sure we should add this.

If we were adding a worker_command kwarg to the OpenStackCluster object and then passing it all the way down the chain then that might make sense. But in this case I would suggest implementing this in a consistent way across all the cluster classes.

Given that you are using OpenStackInstance in a way that is not intended I would highly recommend creating your own subclass and then customising it however you want rather than upstreaming your changes here.

class MyOpenStackInstance(OpenStackInstance):
    def __init__(*args, command=None, **kwargs):
        super().__init__(*args, **kwargs)
        self.command = command

@armagankaratosun
Copy link
Contributor Author

Hey again,

Hmm, I get your point, fair enough. I think adding worker_command kwarg would help others, this is why I also thought that having this change in upstream makes sense.

I should do it that way all the way, as you suggested.

Thanks!

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