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 a bulkdata flag #3893

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Add a bulkdata flag #3893

merged 1 commit into from
Jul 10, 2024

Conversation

timmow
Copy link
Member

@timmow timmow commented Jun 13, 2024

If this is set to false, do not mount the /nail/bulkdata volume

Tested by

  • following instructions on this page

  • editing etc_paasta_playground/volumes.json and adding the bulkdata volume

  • running docker ps | grep kind to find the worker node container ids

  • using these ids, running docker exec -t -i <id> bash and creating the /nail/bulkdata directory on the worker nodes

  • editing config_playground/compute-infra-test-service/kubernetes-kind-tmower-k8s-test.yaml and setting bulkdata: false

  • verifying with KUBECONFIG=./.paasta-kube-config kubectl get deployment -n paastasvc-compute-infra-test-service -oyaml the /nail/bulkdata volume was not added

I'm not entirely sure why black has reformatted the kubernetes_tools.py file

Copy link
Contributor

@Rob-Johnson Rob-Johnson left a comment

Choose a reason for hiding this comment

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

please all add docs for the new yelpsoa-configs key here https://github.com/Yelp/paasta/blob/master/docs/source/yelpsoa_configs.rst#L290 - these docs aren't in a good state, but it'd be good to stop the rot if we can

paasta_tools/kubernetes_tools.py Outdated Show resolved Hide resolved
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

imo, something like include_bulkdata/uses_bulkdata/etc would be a more intuitive name for the config option - but i don't have particularly strong feelings about it if y'all prefer just bulkdata

paasta_tools/kubernetes_tools.py Outdated Show resolved Hide resolved
@timmow timmow force-pushed the u/tmower/opt-out-bulkdata-PERES-4952 branch from 6af1484 to 84917f3 Compare June 24, 2024 15:21
Copy link
Member

@nemacysts nemacysts left a comment

Choose a reason for hiding this comment

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

lgtm - just two minor nits (that are ignorable, but would be nice to handle)

paasta_tools/utils.py Outdated Show resolved Hide resolved
paasta_tools/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Rob-Johnson Rob-Johnson left a comment

Choose a reason for hiding this comment

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

please add a test - realise that there is nothing there atm, but something is better than nothing.

self,
) -> bool:
return self.config_dict.get("uses_bulkdata", True)

def get_volumes(self, system_volumes: Sequence[DockerVolume]) -> List[DockerVolume]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have a test for this right now...could you add one for this function whilst you're here please?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a test that adding the uses_bulkdata flag adds the bulkdata volume. I've had to fix other failing tests by adding the uses_bulkdata flag set to False to prevent the bulkdata volume being added, so I think there is coverage of this method?

If this is set to true, mount the /nail/bulkdata volume
If set to false, do not mount the /nail/bulkdata volume

Because this happens before `reorder_docker_volumes` is called, if the
bulkdata mount is configured in `volumes.json` a duplicate volume will
not be added. It means that if the bulkdata mount is included in
`volumes.json`, this flag will have no effect

Tested by
* following instructions on [this
page](https://yelpwiki.yelpcorp.com/pages/viewpage.action?pageId=234312044)
* editing
  `config_playground/compute-infra-test-service/kubernetes-kind-tmower-k8s-test.yaml`
  and setting bulkdata: true

* verifying with `KUBECONFIG=./.paasta-kube-config kubectl get deployment -n paastasvc-compute-infra-test-service -oyaml` the /nail/bulkdata volume was added
* editing
  `config_playground/compute-infra-test-service/kubernetes-kind-tmower-k8s-test.yaml`
  and setting bulkdata: false

* verifying with `KUBECONFIG=./.paasta-kube-config kubectl get deployment -n paastasvc-compute-infra-test-service -oyaml` the /nail/bulkdata volume was not added
* editing `etc_paasta_playground/volumes.json` and adding the bulkdata
  volume
* running `docker ps | grep kind` to find the worker node container ids
* using these ids, running `docker exec -t -i <id> bash` and creating
  the /nail/bulkdata directory on the worker nodes
* editing
  `config_playground/compute-infra-test-service/kubernetes-kind-tmower-k8s-test.yaml`
  and setting bulkdata: false

* verifying with `KUBECONFIG=./.paasta-kube-config kubectl get deployment -n paastasvc-compute-infra-test-service -oyaml` the /nail/bulkdata volume was stil added (as its in the default volumes)

* editing
  `config_playground/compute-infra-test-service/kubernetes-kind-tmower-k8s-test.yaml`
  and setting bulkdata: true

* verifying with `KUBECONFIG=./.paasta-kube-config kubectl get deployment -n paastasvc-compute-infra-test-service -oyaml` the /nail/bulkdata volume was still added, and there are no duplicate volume warnings

I'm not entirely sure why `black` has reformatted the
`kubernetes_tools.py` file
@timmow timmow force-pushed the u/tmower/opt-out-bulkdata-PERES-4952 branch from 5294596 to 7aa4791 Compare July 4, 2024 16:33
@timmow timmow merged commit 3ed4126 into master Jul 10, 2024
10 checks passed
@timmow timmow deleted the u/tmower/opt-out-bulkdata-PERES-4952 branch July 10, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants