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.

Sign up for GitHub

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

Jump to bottom

TRON-1636: Setup tron secret_volumes in setup_tron_namespace #3615

Merged
merged 3 commits into from
Jul 18, 2023
Merged

TRON-1636: Setup tron secret_volumes in setup_tron_namespace #3615

merged 3 commits into from
Jul 18, 2023

Conversation

Molaire
Copy link
Contributor

@Molaire Molaire commented May 30, 2023 •
edited
Loading

Ticket: TRON-1636

This pipes secret_volumes in tron. It was already set up in kubernetes, so most of the work here was adjusting the tests.

  • Had to use a file-based approach to figure out if the secret is shared or not, as it's not explicit for secret_volumes.
  • Added a TronSecretVolume type to allow passing secret_volume_name to Tron/task_processing, so they don't have to figure that out themselves.
  • Tested with patched wheel of Add support for file-based secrets task_processing#188. So once this task_proc PR is merged, we should update requirements.txt

This was tested with 2 real tron jobs and patched paasta and tron on our tron-infrastage box.
Success: https://fluffy.yelpcorp.com/i/bsJZQ3TlDR9N6fx3dNjT6WQfT0NVGh03.html
Jobs config: https://sourcegraph.yelpcorp.com/sysgit/yelpsoa-configs/-/blob/compute-infra-test-service/tron-infrastage.yaml?L94-119

@Molaire Molaire changed the title Setup tron secret_volumes in setup_tron_namespace TRON-1636: Setup tron secret_volumes in setup_tron_namespace May 30, 2023
@Molaire Molaire requested a review from nemacysts May 30, 2023 20:15
@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from c3060ff to e9ccd71 Compare May 31, 2023 20:49
@Molaire Molaire requested a review from jfongatyelp June 1, 2023 15:37
danielpops
danielpops reviewed Jun 5, 2023
View reviewed changes
unique-run Outdated Show resolved Hide resolved
danielpops
danielpops reviewed Jun 5, 2023
View reviewed changes
tests/test_tron_tools.py
@@ -1430,7 +1535,7 @@ def test_create_complete_config_e2e(self, tmpdir):
# that are not static, this will cause continuous reconfiguration, which
# will add significant load to the Tron API, which happened in DAR-1461.
# but if this is intended, just change the hash.
assert hasher.hexdigest() == "f740410f7ae2794f9924121c1115e15d"
assert hasher.hexdigest() == "35972651618a848ac6bf7947245dbaea"
Copy link
Member

Choose a reason for hiding this comment

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

Note that this may cause a big bounce with all tron jobs when deployed. Make sure you sync with #compute-infra before shipping these changes in case any special considerations are needed to accommodate that.

Copy link
Member

Choose a reason for hiding this comment

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

this should be fine - the main purpose of this test is too help us catch things like a timestamp (or something equally ever-changing) being added to the podspec as repeatedly reconfiguring every job is quite painful :)

Copy link
Member

Choose a reason for hiding this comment

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

luisp++ ah right there in the comment above :) thanks for the context!

@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from 9a0fe06 to ade4b57 Compare June 5, 2023 18:18
nemacysts
nemacysts approved these changes Jun 5, 2023
View reviewed changes
tests/test_tron_tools.py
@@ -1430,7 +1535,7 @@ def test_create_complete_config_e2e(self, tmpdir):
# that are not static, this will cause continuous reconfiguration, which
# will add significant load to the Tron API, which happened in DAR-1461.
# but if this is intended, just change the hash.
assert hasher.hexdigest() == "f740410f7ae2794f9924121c1115e15d"
assert hasher.hexdigest() == "35972651618a848ac6bf7947245dbaea"
Copy link
Member

Choose a reason for hiding this comment

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

this should be fine - the main purpose of this test is too help us catch things like a timestamp (or something equally ever-changing) being added to the podspec as repeatedly reconfiguring every job is quite painful :)

tests/test_tron_tools.py Show resolved Hide resolved
paasta_tools/tron_tools.py
@@ -869,6 +869,7 @@ def format_tron_action_dict(action_config: TronActionConfig, use_k8s: bool = Fal
"node": action_config.get_node(),
"retries": action_config.get_retries(),
"retries_delay": action_config.get_retries_delay(),
"secret_volumes": action_config.get_secret_volumes(),
Copy link
Member

Choose a reason for hiding this comment

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

lol - nice! always good when things just work out and we don't need to do anything special :)

.gitignore
@@ -48,6 +48,7 @@ example_cluster/paasta/docker_registry.json
general_itests/fake_etc_paasta/clusters.json
pip-wheel-metadata
debian/debhelper-build-stamp
unique-run
Copy link
Member

Choose a reason for hiding this comment

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

++

@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from 1b94de5 to a45bf49 Compare June 7, 2023 14:56
@Molaire Molaire marked this pull request as draft June 7, 2023 19:15
@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch 5 times, most recently from e4ace1f to 9ccbea3 Compare June 9, 2023 14:37
@Molaire Molaire marked this pull request as ready for review June 9, 2023 14:38
Molaire
Molaire commented Jun 15, 2023
View reviewed changes
paasta_tools/tron_tools.py
@@ -415,15 +415,17 @@ def get_job_name(self):
def get_action_name(self):
return self.action

def get_secret_volumes(self) -> List[TronSecretVolume]:
def get_secret_volumes(self) -> List[TronSecretVolume]: # type: ignore
Copy link
Contributor Author

@Molaire Molaire Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

TypedDict does not support isinstance and issubclass for a subclass of a defined typeddict, and mypy is sad (mypy doesnt understand TronSecretVolume is a subclass of SecretVolume)

Copy link
Member

Choose a reason for hiding this comment

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

rip, maybe we can unify TronSecretVolume and SecretVolume in the future (if we did it now we'd need another tron release, right?)

@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from 782a764 to 860d925 Compare June 15, 2023 19:50
nemacysts
nemacysts approved these changes Jun 16, 2023
View reviewed changes
requirements.txt Outdated
@@ -104,7 +104,7 @@ sticht==1.1.18
strict-rfc3339==0.7
swagger-spec-validator==2.1.0
syslogmp==0.2.2
task-processing==0.1.3
task-processing==0.12.1
Copy link
Member

Choose a reason for hiding this comment

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

do we actually use task_processing in paasta right now? 😮

Copy link
Member

Choose a reason for hiding this comment

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

It's "used" in the legacy remote run implementation

Copy link
Member

Choose a reason for hiding this comment

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

noice - well, we don't necessarily need to bump this (and remote-run isn't really usable by folks right now), but this is fine to do now imo :)

@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from 860d925 to 3430d4f Compare July 18, 2023 13:20
@Molaire Molaire force-pushed the u/vit/tron-1636-add-secret-volume branch from 1e3d347 to 6e5c9b1 Compare July 18, 2023 20:55
@Molaire Molaire merged commit 348a645 into master Jul 18, 2023
10 of 12 checks passed
@Molaire Molaire deleted the u/vit/tron-1636-add-secret-volume branch July 18, 2023 21:04
@wilmer05 wilmer05 mentioned this pull request Jul 19, 2023
Merged
@Molaire Molaire mentioned this pull request Jul 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemacysts nemacysts nemacysts approved these changes

@danielpops danielpops danielpops approved these changes

@jfongatyelp jfongatyelp Awaiting requested review from jfongatyelp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Molaire @nemacysts @danielpops