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

Changes for in-kind contribution JAP-JPG-S3 from NAOJ #3913

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Changes for in-kind contribution JAP-JPG-S3 from NAOJ #3913

wants to merge 19 commits into from

Conversation

michitaro
Copy link
Collaborator

@michitaro michitaro commented Nov 22, 2024 •
edited
Loading

These changes are for the in-kind contribution JAP-JPG-S3.

I'm discussing the following points with @ktlim :

  • The tile generator should be a deployment instead of daemonset
  • Postgres password should be managed within the phalanx secret framework.

ktlim
ktlim reviewed Nov 22, 2024
View reviewed changes
Copy link
Contributor

@ktlim ktlim left a comment

Choose a reason for hiding this comment

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

There are some things that can be improved and a few things that I think won't work as currently specified. You can try deploying from this branch with https://phalanx.lsst.io/developers/switch-environment-to-branch.html#switch-an-environment-to-a-branch and https://phalanx.lsst.io/developers/deploy-from-a-branch.html#deploying-from-a-branch-for-development; I will approve once things are actually working.

applications/fov-quicklook/secrets.yaml Outdated
@@ -0,0 +1,3 @@
fov-quicklook-secret:
Copy link
Contributor

Choose a reason for hiding this comment

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

The secret is already named fov-quicklook. This file should contain the keys within the secret like s3_tile_access_key and s3_tile_secret_key.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 6b02b2c

applications/fov-quicklook/templates/_helpers.tpl Outdated
- name: QUICKLOOK_s3_tile__access_key
valueFrom:
secretKeyRef:
name: fov-quicklook-secret
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the secret should always be the application name (fov-quicklook).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in c84413f

applications/fov-quicklook/templates/_helpers.tpl Show resolved Hide resolved
applications/fov-quicklook/templates/frontend.yaml Outdated
port: 9500
targetPort: 9500
---
apiVersion: v1
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 65a8419

applications/fov-quicklook/templates/frontend.yaml Outdated
config:
scopes:
all:
- "read:all"
Copy link
Contributor

Choose a reason for hiding this comment

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

read:image is more appropriate, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 551304b

applications/fov-quicklook/templates/coordinator.yaml Outdated
- name: DB_PASSWORD
valueFrom:
secretKeyRef:
name: quicklook-db
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is OK for now. Normally for a phalanx application, this would be still in the application secret (fov-quicklook) and there would be a generate clause in the secrets.yaml for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in f214bf0

applications/fov-quicklook/templates/db.yaml Outdated
initContainers:
- name: init-permissions
image: 'busybox'
command: ['sh', '-c', 'chown -R 999:999 /var/lib/postgresql/data']
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned that this will cause delays on startup. If nothing else is touching the Postgres data files, can this be limited to doing a chown of the mount path only instead of having -R?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in d387624

applications/fov-quicklook/templates/_helpers.tpl Outdated
valueFrom:
secretKeyRef:
name: fov-quicklook-secret
key: s3_repository_acceess_key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
key: s3_repository_acceess_key
key: s3_repository_access_key

Copy link
Collaborator Author

@michitaro michitaro Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fixed in f0d243d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 137df11

applications/fov-quicklook/templates/secret.yaml Outdated
path: "{{ .Values.global.vaultSecretsPathPrefix }}/fov-quicklook"
type: Opaque
keys:
- s3_repository_acceess_key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- s3_repository_acceess_key
- s3_repository_access_key

Copy link
Collaborator Author

@michitaro michitaro Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fixed in f0d243d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 137df11

applications/fov-quicklook/templates/coordinator.yaml
{{- include "fov-quicklook.env.s3_repository" . | nindent 12 }}
ports:
- containerPort: 9501
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

You should provide the ability to set additional information like resource requests and limits in these deployments. Another thing that may be necessary to configure, especially for the generator, is tolerations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in ab67731, c2f5203, 192b0f8

@michitaro michitaro force-pushed the u/michitaro/fov-quicklook branch from 2745217 to 192b0f8 Compare December 3, 2024 01:21
@michitaro
Copy link
Collaborator Author

Thank you for the review.
I have incorporated the comments you provided.

Currently, the generator’s limits.memory is set to 32GiB.
For most of the runtime, memory usage remains at a few GiBs. However, there are moments during processing when it exceeds 16GiB, which is why it is set to 32GiB.
I'm currently investigating the cause of these temporary peaks. Once resolved, the limit can be reduced to around 8GiB.

@michitaro michitaro force-pushed the u/michitaro/fov-quicklook branch from 192b0f8 to 826e75d Compare December 10, 2024 04:35
@michitaro
Copy link
Collaborator Author

Add settings for butler in 826e75d.

michitaro added 19 commits January 8, 2025 10:24
@michitaro
draft for fov-quicklook
255bbd9
@michitaro
typo
4df4758
@michitaro
Reorder applications alphabetically
6383001
@michitaro
Fix typo
cc9b8a0
@michitaro
rename secret name fov-quicklook{-secret,}
109c0d4
@michitaro
update secrets
75329da
@michitaro
make object storage bucket configurable
8780777
@michitaro
make db_storage_class non-mandatory
10da8e9
@michitaro
remove duplicated definition of service
e4721c4
@michitaro
update frontend.yaml to restrict read scope to 'read:image'
11c472c
@michitaro
Rename resources: quicklook -> fov-quicklook
f7bfeb3
@michitaro
merge secret fov-quicklook-db into secret fov-quicklook
7712cec
@michitaro
remove unnecessary '-r' from chwon
061f41c
@michitaro
Add resource requests and limits for coordinator, frontend, db, and g…
d793ad0 
…enerator
@michitaro
Add workdir configuration to generator with medium option
01b2b2e
@michitaro
update resources.limits.memory to 32GiB
701fb1e
@michitaro
Add generator replicas configuration and update deployment strategy
8cd52a1
@michitaro
add settings for butler
53ff699
@michitaro
Add AWS and PostgreSQL credentials to secret.yaml
1d68dc2
@michitaro michitaro force-pushed the u/michitaro/fov-quicklook branch from e9b0639 to 1d68dc2 Compare January 8, 2025 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktlim ktlim ktlim left review comments

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

Successfully merging this pull request may close these issues.
2 participants
@michitaro @ktlim