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

fix(BaseNode): change directory for startup script upload from /tmp to $HOME. #9608

Merged
merged 1 commit into from
Dec 24, 2024
Merged

fix(BaseNode): change directory for startup script upload from /tmp to $HOME. #9608

merged 1 commit into from
Dec 24, 2024

Conversation

mikliapko
Copy link
Contributor

@mikliapko mikliapko commented Dec 23, 2024 •
edited
Loading

Fix changes directory for startup script upload from /tmp to $HOME.
The change is required for DB nodes deployed in Cloud where /tmp dir is mounted with noexec option what makes script
execution impossible there.

Testing

  • siren-sct-integration test

PR pre-checks (self review)

  • I added the relevant backport labels
  • I didn't leave commented-out/debugging code

@mikliapko mikliapko added the backport/none Backport is not required label Dec 23, 2024
@mikliapko mikliapko self-assigned this Dec 23, 2024
@fruch
Copy link
Contributor

fruch commented Dec 24, 2024 •
edited
Loading

@mikliapko what's the implementation in cloud ? using the $HOME ?

maybe we can just switch to it ?, there's nothing holy in /tmp for SCT, we just need to run the script.

@mikliapko
Copy link
Contributor Author

@mikliapko what's the implementation in cloud ? using the $HOME ?

maybe we can just switch to it ?, there's nothing holy in /tmp for SCT, we just need to run the script.

Yes, the plan is to use $HOME in cloud nodes.
If the directory doesn't matter for SCT as you mentioned above, I will just switch to $HOME in SCT without introducing new attributes.

@fruch
Copy link
Contributor

fruch commented Dec 24, 2024

@mikliapko what's the implementation in cloud ? using the $HOME ?
maybe we can just switch to it ?, there's nothing holy in /tmp for SCT, we just need to run the script.

Yes, the plan is to use $HOME in cloud nodes. If the directory doesn't matter for SCT as you mentioned above, I will just switch to $HOME in SCT without introducing new attributes.

I think it would be better, the defining yet another "API" like that between the two parts

@mikliapko
fix(BaseNode): change dir for startup script from /tmp to $HOME
d16caab 
Fix changes directory for startup script upload from /tmp to $HOME.
The change is required for DB nodes deployed in Cloud where /tmp dir
is mounted with noexec option what makes script execution impossible
there.

As per discussion (#1), for DB nodes deployed in SCT startup_script
can be executed either from $HOME or /tmp.

refs:
#1: scylladb#9608
@mikliapko mikliapko force-pushed the setup-script-path-to-attrs branch from 0d695f4 to d16caab Compare December 24, 2024 13:31
@mikliapko
Copy link
Contributor Author

I think it would be better, the defining yet another "API" like that between the two parts

Changed to $HOME, testing here.

@fruch
Which SCT may I run to validate changes don't lead to any regression?

@fruch fruch added test-provision-aws Run provision test on AWS New Hydra Version PR# introduces new Hydra version labels Dec 24, 2024
@fruch
Copy link
Contributor

fruch commented Dec 24, 2024

I think it would be better, the defining yet another "API" like that between the two parts

Changed to $HOME, testing here.

@fruch
Which SCT may I run to validate changes don't lead to any regression?

Running CI with AWS provision test, I think that should suffice

@mikliapko mikliapko marked this pull request as ready for review December 24, 2024 14:51
@mikliapko mikliapko requested a review from a team December 24, 2024 14:53
@mikliapko mikliapko mentioned this pull request Dec 24, 2024
Open
@mikliapko mikliapko changed the title fix(BaseNode): define startup_script_remote_path as a class attributes fix(BaseNode): change directory for startup script upload from /tmp to $HOME. Dec 24, 2024
vponomaryov
vponomaryov approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@vponomaryov vponomaryov left a comment

Choose a reason for hiding this comment

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

LGTM

fruch
fruch approved these changes Dec 24, 2024
View reviewed changes
Copy link
Contributor

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

The provision test is failing on nemesis, unrelated to this change

@fruch fruch merged commit 2940923 into scylladb:master Dec 24, 2024
8 of 10 checks passed
mikliapko added a commit that referenced this pull request Dec 27, 2024
@mikliapko
fix(BaseNode): change dir for startup script from /tmp to $HOME
b8b1080 
Fix changes directory for startup script upload from /tmp to $HOME.
The change is required for DB nodes deployed in Cloud where /tmp dir
is mounted with noexec option what makes script execution impossible
there.

As per discussion (#1), for DB nodes deployed in SCT startup_script
can be executed either from $HOME or /tmp.

refs:
#1: #9608
@mikliapko mikliapko deleted the setup-script-path-to-attrs branch December 27, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vponomaryov vponomaryov vponomaryov approved these changes

@fruch fruch fruch approved these changes

Assignees

@mikliapko mikliapko

Labels
backport/none Backport is not required New Hydra Version PR# introduces new Hydra version promoted-to-master test-provision-aws Run provision test on AWS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mikliapko @fruch @vponomaryov @scylladbbot