Skip to content

mgr/cephadm: make jaeger-collector a dep for jaeger-agent#51416

Merged
adk3798 merged 1 commit intoceph:mainfrom
adk3798:jaeger-agent-dep
Oct 12, 2023
Merged

mgr/cephadm: make jaeger-collector a dep for jaeger-agent#51416
adk3798 merged 1 commit intoceph:mainfrom
adk3798:jaeger-agent-dep

Conversation

@adk3798
Copy link
Contributor

@adk3798 adk3798 commented May 9, 2023

the jaeger-agent's need to build a url for the collector(s) that have been deployed. If a collector moves, or we deployed the agents before the collector, we need to reconfig the agents with updated info about the collectors. Failure to do so can leave the jager-agents down reporting

Could not create collector proxy","error":"at least one collector hostPort address is required when resolver is not available"

Fixes: https://tracker.ceph.com/issues/59704

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@adk3798 adk3798 requested a review from a team as a code owner May 9, 2023 19:18
adk3798
adk3798 commented May 28, 2023
View reviewed changes
@adk3798 adk3798 force-pushed the jaeger-agent-dep branch from 7c52d05 to 72c348e Compare May 28, 2023 02:29
@github-actions
Copy link

github-actions bot commented Sep 27, 2023

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Sep 27, 2023
@adk3798
Copy link
Contributor Author

adk3798 commented Oct 2, 2023

https://pulpito.ceph.com/adking-2023-10-01_22:13:42-orch:cephadm-wip-adk-testing-2023-10-01-1310-distro-default-smithi/7408480

This patch isn't working currently because the config changes we care about only affect the way the unit.run is written, which we don't rewrite when reconfiguring a daemon. We need to make this trigger a full redeploy, not a reconfiguration.

@adk3798
mgr/cephadm: make jaeger-collector urls a dep for jaeger-agent
acc26d7 
the jaeger-agent's need to know the url for the collector(s)
that have been deployed. If a collector moves, or we deployed
the agents before the collector, we need to reconfig the agents
with updated info about the collectors. Failure to do so can
leave the jager-agents down reporting

```
Could not create collector proxy","error":"at least one collector hostPort address is required when resolver is not available"
```

Fixes: https://tracker.ceph.com/issues/59704

Signed-off-by: Adam King <adking@redhat.com>
@adk3798
Copy link
Contributor Author

adk3798 commented Oct 2, 2023

https://pulpito.ceph.com/adking-2023-10-01_22:13:42-orch:cephadm-wip-adk-testing-2023-10-01-1310-distro-default-smithi/7408480

This patch isn't working currently because the config changes we care about only affect the way the unit.run is written, which we don't rewrite when reconfiguring a daemon. We need to make this trigger a full redeploy, not a reconfiguration.

Modified patch to have jaeger-agent be redeployed when its deps change

@adk3798 adk3798 removed the stale label Oct 2, 2023
@adk3798
Copy link
Contributor Author

adk3798 commented Oct 5, 2023

https://pulpito.ceph.com/adking-2023-10-05_01:34:16-orch:cephadm-wip-adk-testing-2023-10-04-1525-distro-default-smithi/

reruns of failed jobs: https://pulpito.ceph.com/adking-2023-10-05_12:25:45-orch:cephadm-wip-adk-testing-2023-10-04-1525-distro-default-smithi/

After reruns, 1 failure

Nothing to block merging

Also, for this Pr in particular, the jaeger-tracing deploying test finally passed https://pulpito.ceph.com/adking-2023-10-05_01:34:16-orch:cephadm-wip-adk-testing-2023-10-04-1525-distro-default-smithi/7411242 so this seems to be working.

@adk3798
Copy link
Contributor Author

adk3798 commented Oct 5, 2023

jenkins retest this please

phlogistonjohn
phlogistonjohn approved these changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

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

looks OK to me.

@adk3798
Copy link
Contributor Author

adk3798 commented Oct 12, 2023

https://pulpito.ceph.com/adking-2023-10-12_12:04:04-orch:cephadm-wip-adk-testing-2023-10-11-2128-distro-default-smithi/

1 failure: https://tracker.ceph.com/issues/62958, known issue

Nothing to block merging

@adk3798 adk3798 merged commit b15daa4 into ceph:main Oct 12, 2023
@adk3798 adk3798 mentioned this pull request Mar 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

Assignees

No one assigned

Labels

cephadm pybind wip-adk-testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adk3798 @phlogistonjohn