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

add missing cases of openshift deployment #158

Merged
merged 3 commits into from
Sep 13, 2023
Merged

add missing cases of openshift deployment #158

merged 3 commits into from
Sep 13, 2023

Conversation

sunya-ch
Copy link
Contributor

@sunya-ch sunya-ch commented Sep 11, 2023 •
edited
Loading

This PR is to fix the issue mentioned in #157.
Also, this PR add make manifest to validate manifest before run make deploy.

Example output:

> OPTS="SERVER OPENSHIFT_DEPLOY" make manifest
SERVER OPENSHIFT_DEPLOY
WARNING: version difference between client (1.23) and server (1.25) exceeds the supported minor version skew of +/-1
Preparing manifests...
deploy model server
patch openshift deployment for exporter (serve-only)
patch openshift deployment for server
Done ./manifests/set.sh
...
> OPTS="ESTIMATOR OPENSHIFT_DEPLOY" make manifest
ESTIMATOR OPENSHIFT_DEPLOY
WARNING: version difference between client (1.23) and server (1.25) exceeds the supported minor version skew of +/-1
Preparing manifests...
add estimator-sidecar
patch openshift deployment for exporter (estimator-only)
Done ./manifests/set.sh
...
>  OPTS="ESTIMATOR SERVER OPENSHIFT_DEPLOY" make manifest
ESTIMATOR SERVER OPENSHIFT_DEPLOY
WARNING: version difference between client (1.23) and server (1.25) exceeds the supported minor version skew of +/-1
Preparing manifests...
deploy model server
add estimator-sidecar
patch openshift deployment for exporter (estimator-with-server)
patch openshift deployment for server
Done ./manifests/set.sh
...

[edit] Add fix #160.

Signed-off-by: Sunyanan Choochotkaew sunyanan.choochotkaew1@ibm.com

BGrasnick
BGrasnick reviewed Sep 11, 2023
View reviewed changes
manifests/base/openshift/scc.yaml Outdated
Comment on lines 7 to 12
allowPrivilegedContainer: true
allowHostDirVolumePlugin: true
allowHostNetwork: true
allowHostPorts: true
allowHostIPC: true
allowHostPID: true
Copy link

Choose a reason for hiding this comment

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

Hey, as a best practice I would actually suggest restricting the kepler pods a little bit more as done lately in the kepler-operator as seen here.

I still need to raise a PR to also integrate this also into the main kepler repo but I think it would be good to already do this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BGrasnick Thank you so much for the review. Agree. Just update the scc file, please confirm.

Copy link

Choose a reason for hiding this comment

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

Yes perfect, LGTM now!

BGrasnick
BGrasnick reviewed Sep 11, 2023
View reviewed changes
manifests/base/patch/patch-openshift.yaml Outdated
Comment on lines 22 to 30
- name: kernel-debug
mountPath: /sys/kernel/debug
securityContext:
privileged: true
volumes:
- name: kernel-debug
hostPath:
path: /sys/kernel/debug
type: Directory
Copy link

Choose a reason for hiding this comment

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

There is still an open discussion if mounting /sys/kernel/debug is actually needed for OpenShift. In my local tests everything seemed to work perfectly fine and normally when excluding it. See the discussion here and the PR sustainable-computing-io/kepler-operator#198. Maybe you can test this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe/sys/kernel/debug is still needed for libbpf to access the tracepoint files such as /sys/kernel/debug/tracing/events/irq/softirq_entry. Have you tried deploying with latest-libbpf tag?

Copy link

Choose a reason for hiding this comment

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

Yes I believe I tested both. Can probably try again later today or tomorrow! As I am no expert though maybe somebody else with more knowledge about libbpf could test as well? Don't know who that is though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am one of the team who works on this.
However, I just recheck the manifest and found that there is no need to add the mount /sys/kernel/debug here because the /sys is already attached by kepler manifest and the path is the same (i.e., /sys/kernel/debug). Will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BGrasnick Thanks again.
Made a rebase and update the change to open shift-patch.

@sunya-ch sunya-ch force-pushed the setup-manifest branch 3 times, most recently from e4a3589 to 13b5abf Compare September 12, 2023 07:26
@sunya-ch
Copy link
Contributor Author

As it is related to manifest, I also add fix for the issue #160 to this PR.

@sunya-ch
add missing cases of openshift deployment
91ae466 
Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>
@sunya-ch
move download path to under mnt
b01e5ac 
Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>
@sunya-ch
remove /sys/kernel/debug from openshift patch
f26c311 
Signed-off-by: Sunyanan Choochotkaew <sunyanan.choochotkaew1@ibm.com>
@BGrasnick BGrasnick mentioned this pull request Sep 12, 2023
Merged
marceloamaral
marceloamaral approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@marceloamaral marceloamaral left a comment

Choose a reason for hiding this comment

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

/lgtm

@sunya-ch sunya-ch merged commit aa80bdc into sustainable-computing-io:main Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BGrasnick BGrasnick BGrasnick left review comments

@marceloamaral marceloamaral marceloamaral approved these changes

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

Successfully merging this pull request may close these issues.

Need to fix mount path for read-only system
3 participants
@sunya-ch @BGrasnick @marceloamaral