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

Display logmessage if two nodes run on different versions #8088

Merged
merged 1 commit into from Jan 11, 2021
Merged

Display logmessage if two nodes run on different versions #8088

merged 1 commit into from Jan 11, 2021

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Jul 6, 2020

resolves #8075

@yhabteab yhabteab requested a review from Al2Klimov July 6, 2020 15:34
Al2Klimov
Al2Klimov reviewed Jul 6, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from Al2Klimov July 6, 2020 15:47
Al2Klimov
Al2Klimov requested changes Jul 6, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from Al2Klimov July 6, 2020 15:58
Al2Klimov
Al2Klimov requested changes Jul 6, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from Al2Klimov July 6, 2020 16:05
Al2Klimov
Al2Klimov requested changes Jul 6, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

You're not checking whether the node is your child/parent/...

@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 08:52
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Just for testing:

Set the master version to 21204 and the sat one to 21102.

What happens?

@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 09:48
@yhabteab yhabteab force-pushed the bugfix/log-two-nodes-run-on-different-versions-8075 branch from a18b8ac to ae12f12 Compare July 7, 2020 09:50
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

You're not checking whether the child runs a later version.

@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 10:00
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

You're printing versions like 212. Print them like 2.12.x instead.

@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 12:20
@yhabteab yhabteab force-pushed the bugfix/log-two-nodes-run-on-different-versions-8075 branch from a61bc51 to ccd0c98 Compare July 7, 2020 13:00
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

You have some duplicate code.

Make a (non-class) function above this one (prefix the function definition with the "static" keyword) taking an unsigned long (e.g. 212) and a Log& and writing "2.12.x" into the given Log&.

@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 13:29
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 14:01
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
lib/remote/apilistener.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 14:11
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment •
edited by yhabteab

Choose a reason for hiding this comment

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

  • git reset --soft 3f22bebf2
  • git commit --amend --no-edit
  • git push -f origin bugfix/log-two-nodes-run-on-different-versions-8075

@yhabteab yhabteab force-pushed the bugfix/log-two-nodes-run-on-different-versions-8075 branch from 19be712 to 83567ab Compare July 7, 2020 14:15
@yhabteab yhabteab requested a review from Al2Klimov July 7, 2020 14:15
Al2Klimov
Al2Klimov requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Write a test protocol.

Cover all valid cases and two invalid (too high/too low).

@yhabteab
Copy link
Member Author

yhabteab commented Jul 7, 2020 •
edited

Testprotocol:
So far it was the problem that if a node runs on 2 versions behind or 1 version before its parent, you could not see any log messages that could indicate this. But this bug has now been fixed and I will illustrate it to you as follows.

Let us assume that the node is version 2.10.x and the master 2.12.x should then see a log message, because it is not allowed to use node that is 2 versions behind master.

Version of Master v2.12.0 and version of Node v2.10.0.
Output:

warning/ApiListener: Unexpected Icinga version of endpoint 'sat': 2.10.x  Expected one of: 2.12.x, 2.11.x

However, if the node has the same version as the master or is 1 version behind the master, we do not need a log message because it is allowed.

The master can be 1 version before the node, but the node cannot be 1 version before the master.

Version of Master v2.12.0 and version of Node v2.13.0:

[2020-07-07 16:40:39 +0200] warning/ApiListener: Unexpected Icinga version of endpoint 'sat': 2.13.x . Expected one of: 2.12.x, 2.11.x

@Al2Klimov Al2Klimov added the needs feedback We'll only proceed once we hear from you again label Jul 9, 2020
@lazyfrosch
Copy link
Contributor

Uhm sorry not my expertise

@lazyfrosch lazyfrosch assigned Al2Klimov and unassigned lazyfrosch Jul 9, 2020
@Al2Klimov Al2Klimov removed their assignment Jul 9, 2020
@Al2Klimov Al2Klimov removed the needs feedback We'll only proceed once we hear from you again label Jul 9, 2020
@lazyfrosch
Copy link
Contributor

GCC bugs can occur, especially with older maintained versions. We should find a workaround to avoid the problem.

@Al2Klimov
Copy link
Member

This is likely #7149.

@Al2Klimov Al2Klimov self-assigned this Jul 10, 2020
@Al2Klimov Al2Klimov moved this from To review to Blocked in v2.13.0 merge window Jul 10, 2020
@Al2Klimov Al2Klimov removed their assignment Jul 10, 2020
@Al2Klimov
Copy link
Member

Al2Klimov commented Jul 10, 2020 •
edited

Blocked by

@Al2Klimov Al2Klimov marked this pull request as draft July 15, 2020 08:27
@Al2Klimov Al2Klimov marked this pull request as ready for review December 14, 2020 14:27
Al2Klimov
Al2Klimov requested changes Dec 14, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Rebase with -i. Keep only your commit.

v2.13.0 merge window automation moved this from Blocked to Changes requested Dec 14, 2020
@yhabteab yhabteab force-pushed the bugfix/log-two-nodes-run-on-different-versions-8075 branch 2 times, most recently from 7fe7c94 to d5aebf8 Compare December 16, 2020 14:20
@yhabteab yhabteab force-pushed the bugfix/log-two-nodes-run-on-different-versions-8075 branch from d5aebf8 to 8eb4f2e Compare December 16, 2020 15:09
Al2Klimov
Al2Klimov requested changes Dec 16, 2020
View reviewed changes
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

Re-test #8088 (comment) (just to be sure).

@yhabteab
Copy link
Member Author

Tested it and it works fine!

[2021-01-11 08:51:25 +0100] information/ApiListener: Finished sending config file updates for endpoint 'satellite' in zone 'satellite'.
[2021-01-11 08:51:25 +0100] warning/ApiListener: Unexpected Icinga version of endpoint 'satellite': 2.10.x Expected one of: 2.12.x, 2.11.x

v2.13.0 merge window automation moved this from Changes requested to Approved Jan 11, 2021
@Al2Klimov Al2Klimov merged commit 635a8c5 into master Jan 11, 2021
v2.13.0 merge window automation moved this from Approved to Merged Jan 11, 2021
@icinga-probot icinga-probot bot deleted the bugfix/log-two-nodes-run-on-different-versions-8075 branch January 11, 2021 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Al2Klimov Al2Klimov Al2Klimov approved these changes

Assignees
No one assigned
Labels
area/distributed Distributed monitoring (master, satellites, clients) area/log Logging related enhancement New feature or request
Projects
No open projects
v2.13.0 merge window
  
Merged
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

Log if two corresponding nodes run on different versions of Icinga 2
3 participants
@yhabteab @Al2Klimov @lazyfrosch