Landscape Client

Merge lp://staging/~jkakar/landscape-client/silence-repeating-eucaconf-errors into lp://staging/~landscape/landscape-client/trunk

  1. silence-repeating-eucaconf-errors
  2. Merge into trunk
Proposed by Jamu Kakar
Status: Merged
Approved by: Thomas Herve
Approved revision: 253
Merged at revision: 251
Proposed branch: lp://staging/~jkakar/landscape-client/silence-repeating-eucaconf-errors
Merge into: lp://staging/~landscape/landscape-client/trunk
Diff against target: 93 lines (+34/-3)
2 files modified
landscape/manager/eucalyptus.py (+6/-1)
landscape/manager/tests/test_eucalyptus.py (+28/-2)
To merge this branch: bzr merge lp://staging/~jkakar/landscape-client/silence-repeating-eucaconf-errors
Reviewer Review Type Date Requested Status
Thomas Herve (community) Approve
Free Ekanayaka (community) Approve
Review via email: mp+26536@code.staging.launchpad.net

Description of the change

This branch introduces the following changes:

- If an error occurs when the Eucalyptus plugin runs, the plugin is
  disabled. This will prevent it from queueing tons of error
  messages to send to the server, and also from filling the client
  log with repeated error messages.

To post a comment you must log in.
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

[1]

It's not related to this branch but I think that the "data_path =" assignment should be taken out of the try block here:

        try:
            data_path = self.registry.config.data_path
            service_hub = self._service_hub_factory(data_path)

[2]

+ self.enabled = False

I'm a bit concerned that this could catch some false positive, meaning that a random temporary failure in processing the GetEucaInfo task.

Revision history for this message
Jamu Kakar (jkakar) wrote :

Free:

[1]

Fixed, thanks.

[2]

I thought about this issue, too. I'm not entirely sure the change
I've made is totally sane. One thing we could do is catch
EucaToolsError and, only in that case, disable the plugin.
EucaToolsError is raised if the image store code can't get
Eucalyptus credentials, for example. The reason I didn't do that
was because I couldn't really think of a realistic intermittent
error the plugin could recover from.

lp://staging/~jkakar/landscape-client/silence-repeating-eucaconf-errors updated
252. By Jamu Kakar

- Move code outside a try/except block (free-1).

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

[2]

I think we should be paranoid and keep us on the safe side. Disabling the plugin only when we catch an error that proves with reasonable confidence that we're not running on UEC node sounds good to me. I the worst case scenario of flooding the logs is still better than disabling the plugin accidentally when it shouldn't.

lp://staging/~jkakar/landscape-client/silence-repeating-eucaconf-errors updated
253. By Jamu Kakar

- Only disable the Eucalyptus plugin if a EucaToolsError is raised
  when its run.

Revision history for this message
Jamu Kakar (jkakar) wrote :

Free:

I've updated the logic to only disable the plugin if a
EucaToolsError is raised when its run.

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Thanks Jamu, looks good to me, +1!

review: Approve
Revision history for this message
Thomas Herve (therve) wrote :

Looks good, +1!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: