Skip to content

fix: validation fails causing etcd events not to be handled correctly#11268

Merged
moonming merged 8 commits into
apache:masterfrom
bzp2010:fix-etcd-sync-logic
May 21, 2024
Merged

fix: validation fails causing etcd events not to be handled correctly#11268
moonming merged 8 commits into
apache:masterfrom
bzp2010:fix-etcd-sync-logic

Conversation

@bzp2010

@bzp2010 bzp2010 commented May 20, 2024

Copy link
Copy Markdown
Contributor

Description

Fixes #11267

Ensure that all etcd events are handled, rather than jumping out of the execution flow when a checking error is encountered.

Current wrong behavior will cause changes in etcd to no longer take effect on the started data plane.

The issue has existed since at least APISIX 2.0, which was long, 4-5 years ago.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@bzp2010 bzp2010 added the bug Something isn't working label May 20, 2024
@bzp2010 bzp2010 self-assigned this May 20, 2024
@bzp2010 bzp2010 changed the title fix: ensure that all etcd events are handled fix: validation fails causing etcd events not to be handled correctly May 20, 2024
@bzp2010
bzp2010 marked this pull request as ready for review May 20, 2024 19:13
## }
## }
## After check, it only appears when watch recovers and returns events in bulk.
cat logs/error.log | grep "}, {" || (echo "failed: Log case 2 unexpected"; exit 1)

@nic-6443 nic-6443 May 21, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

assert failed to check item data in error log too.

@shreemaan-abhishek shreemaan-abhishek May 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

test needs to fail if expectations are not met || gives a exit code 0 which is not detected as a failure by the cli testing suite.

EDIT: my bad, didn't notice the exit 1

@bzp2010 bzp2010 May 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shreemaan-abhishek Actually, the error doesn't cause the test suite to exit either, I'm not sure what's happening. But I've checked the log manually and it has no errors.

I.e. even if it exits with exit 1, the CLI test doesn't fail, which is too strange and should be checked and fixed. But not in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nic-6443 Good catch! Updated.

Comment thread t/cli/test_etcd_sync_event_handle.sh Outdated
cat logs/error.log | grep "}, {" || (echo "failed: Log case 2 unexpected"; exit 1)

# Clean up
make stop

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this last make stop is not needed, it is already run by default. Running twice give this error that pollutes the test output log.

image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shreemaan-abhishek Fixed, that because I add the first line . ./ci/common.sh finally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: error in checking schema with etcd change event handling

5 participants