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.

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

Update monitoring and logging addon with template generation #792

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

w13915984028
Copy link
Member

@w13915984028 w13915984028 commented Aug 7, 2024

Problem:

harvester/harvester#6289

Solution:

  1. Update template generation
  2. Check image tag

Related Issue:
harvester/harvester#6289

Test plan:

Peer PR: harvester/addons#14 those 2 PRs can be merged independently.

These additional ENVs will be appended to the version_info file when building, no matter addon side PR harvester/addons#14 is merged or not.

RANCHER_LOGGING_CHART_VERSION="103.1.0+up4.4.0"
RANCHER_MONITORING_CHART_VERSION="103.1.1+up45.31.1"
HARVESTER_EVENTROUTER_FULL_TAG="rancher/harvester-eventrouter:v0.3.1"

build output:

logging chart version is updated to 103.1.0+up4.4.0
monitoring chart version is updated to 103.1.1+up45.31.1
harvester-eventrouter image tag is updated to rancher/harvester-eventrouter:v0.3.1
#!/bin/bash
VM_IMPORT_CONTROLLER_CHART_VERSION="0.4.0"
VM_IMPORT_CONTROLLER_IMAGE="rancher/harvester-vm-import-controller:v0.4.0"
PCIDEVICES_CONTROLLER_CHART_VERSION="0.4.0"
PCIDEVICES_CONTROLLER_IMAGE="rancher/harvester-pcidevices:v0.4.0"
HARVESTER_SEEDER_CHART_VERSION="0.3.0"
HARVESTER_SEEDER_IMAGE="rancher/harvester-seeder:v0.3.0"
NVIDIA_DRIVER_RUNTIME_CHART_VERSION="0.1.1"
RANCHER_LOGGING_CHART_VERSION="103.1.0+up4.4.0"
RANCHER_MONITORING_CHART_VERSION="103.1.1+up45.31.1"
HARVESTER_EVENTROUTER_FULL_TAG="rancher/harvester-eventrouter:v0.3.1"
the rendered addon template
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "0.4.0"
      chart: harvester-vm-import-controller
--
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "0.4.0"
      chart: harvester-pcidevices-controller
--
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "103.1.0+up4.4.0"
      chart: rancher-logging
--
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "103.1.1+up45.31.1"
      chart: rancher-monitoring
--
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "0.3.0"
      chart: harvester-seeder
--
      repo: http://harvester-cluster-repo.cattle-system.svc/charts
      version: "0.1.1"
      chart: nvidia-driver-runtime

Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

I found a couple of little nits (see below). The main thing I'm wondering though is, it appears that LOGGING_VERSION, MONITORING_VERSION and HARVESTER_EVENTROUTER_FULL_TAG defined in harvester-installer will always override whatever values are set in the addons repo. Would it not be better to instead have harvester-installer take those values from addons version_info, treating that file as the single source of truth, and remove scripts/version-logging and scripts-version-monitoring from harvester-installer?

scripts/build-bundle Outdated Show resolved Hide resolved
scripts/build-bundle Outdated Show resolved Hide resolved
@w13915984028
Copy link
Member Author

@tserong A good question, why not use addon as a single source:

There are some patches on path https://github.com/harvester/harvester-installer/tree/master/pkg/config/templates/patch, which are related to monitoring and logging. And the monitoring and logging ENVs are also used in some other files.

package-harvester-os:source ${SCRIPTS_DIR}/version-monitoring
build:source ${SCRIPTS_DIR}/version-monitoring

I will spend another time to finally move them all to a single place if possible. thanks.

@w13915984028 w13915984028 requested a review from tserong August 8, 2024 07:25
Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

Thanks @w13915984028 !

@w13915984028 w13915984028 requested a review from bk201 August 9, 2024 19:07
scripts/build-bundle Outdated Show resolved Hide resolved
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

lgtm, just a minor nit.

@w13915984028 w13915984028 merged commit e80ee9a into harvester:master Aug 13, 2024
7 checks passed
@tserong
Copy link
Contributor

tserong commented Aug 16, 2024

@Mergifyio backport v1.4

Copy link

mergify bot commented Aug 16, 2024

backport v1.4

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 16, 2024
Signed-off-by: Jian Wang <[email protected]>
(cherry picked from commit e80ee9a)

# Conflicts:
#	scripts/build-bundle
tserong pushed a commit that referenced this pull request Aug 16, 2024
tserong pushed a commit that referenced this pull request Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants