-
Notifications
You must be signed in to change notification settings - Fork 147
DEVPROD-8413 Make ps logging configurable #9750
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
Conversation
agent/agent.go
Outdated
| opts.expansionsAndVars.Expansions.Put("ps", "ps") | ||
| } | ||
| // Project and task settings can override the default. | ||
| if opts.project.Ps != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we either make this PS, or elsewhere have the flag be Ps?
model/project.go
Outdated
| if bvt.Stepback == nil { | ||
| bvt.Stepback = pt.Stepback | ||
| } | ||
| if bvt.Ps == nil && pt.Ps != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the task version of this to also be a pointer, such that it can override behavior? (Not sure if that actually happens, seems consistent with the bool flags though)
config_serviceflags.go
Outdated
| debugSpawnHostDisabledKey: c.DebugSpawnHostDisabled, | ||
| s3LifecycleSyncDisabledKey: c.S3LifecycleSyncDisabled, | ||
| useGitForGitHubFilesDisabledKey: c.UseGitForGitHubFilesDisabled, | ||
| psLoggingDisabledKey: c.PSLoggingDisabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we aren't putting this on the admin page, then we probably don't want to set this here, since we'll just override whatever is set if we make any other changes. (If its temporary it could be okay to just set via DB)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already added it to graphql (in this PR) and I plan to open a UI pr to add it to the admin page. It won't add too much work and it'll make the migration easier.
agent/agent.go
Outdated
| "uptime", | ||
| "df -h", | ||
| "${ps|ps}", | ||
| // The empty fallback is needed to make ps opt-in so that if thee ps expansion is not set, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: thee -> the
agent/agent.go
Outdated
| opts.expansionsAndVars.Expansions.Put("ps", opts.project.Ps) | ||
| } | ||
| if projectTask := opts.project.FindProjectTask(opts.task.DisplayName); projectTask != nil && projectTask.Ps != "" { | ||
| opts.expansionsAndVars.Expansions.Put("ps", projectTask.Ps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're using expansions for this feature, it would also make sense to add this to the Default Expansions section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick update: After some more testing and investigation into our current configurations (I can explain more offline) I decided to refactor this to not use expansions because I want to stop defaulting to distro level ps expansions unless specifically specified with ps: ${ps}. I'm working on that refactor and will re-request review once it's ready.
agent/agent.go
Outdated
| // parameters, so load them from the project. | ||
| // If PSLoggingDisabled is false, default to "ps" command. | ||
| if !a.opts.SetupData.PSLoggingDisabled { | ||
| opts.expansionsAndVars.Expansions.Put("ps", "ps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can the "ps" expansion be a constant? Also perhaps this section can be modularized out to a helper.
| ```yaml | ||
| tasks: | ||
| - name: task_with_custom_ps | ||
| ps: "ps -o pid,tty,time,comm,args" # Custom ps command for this task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if i missed where we did this, but can we add these to the list of available task and variant fields in the docs as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have a consolidated list of task fields (I can create a ticket to add it if you'd like).
The Build Variants Fields section is for fields that can be set directly on the build variant itself (like name, display_name, run_on, etc). This can only be set on the BuildVariantTaskUnit, so I instead added a mention to the tasks field in that list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha. What's the logic for not having this set at the variant level, out of curiosity, just not needed, or bc we want to reduce how often we're doing overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love the big new example also!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to reduce how often we're doing overrides, we can add it later if there is interest.
| } | ||
|
|
||
| // For backward compatibility: when PSLoggingDisabled=false, fall back to ps expansion. | ||
| // This allows distro/build variant expansions to work for existing projects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait sorry just to clarify my understanding -- when we do disable ps logging, does that mean we won't be accepting distro-level expansions anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, once it's disabled, there will be no more automatic ps logging, including not accepting distro-level expansions. Users will have to set ps to ${ps | ps} to get what the default is now.
model/project_parser.go
Outdated
| GitTagOnly: pt.GitTagOnly, | ||
| Stepback: pt.Stepback, | ||
| MustHaveResults: pt.MustHaveResults, | ||
| Ps: pt.Ps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we also change this to be PS to be consistent with Project?
DEVPROD-8413
Description
This PR introduces two things:
psautomatically and it will only run if specified (as described above).Testing
This was tested on staging:
ps.ps: "ps aux"at the task level.UI PR
1336