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

Remove use_carrier bond for Ubuntu Xenial later #34779

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

pupapaik
Copy link
Contributor

@pupapaik pupapaik commented Jul 19, 2016

network.managed type bond generate use_carrier
option, which is unknown parameter in ubuntu 16.04

Fixes #34778

bond.update({'use_carrier': '1'})
elif opts['use_carrier'] in _CONFIG_FALSE:
bond.update({'use_carrier': '0'})
if __grains__['os'] == "Ubuntu" and __grains__['osrelease_info'][0] >= 16:
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean for this to be <= 16 instead of >=? As this stands, this will cause the if 'use_carrier' in opts block to be skipped for older versions of Ubuntu, which is a change in behavior that I don't think you're intending based on your description of the issue.

I am also curious if this block is used for Debian boxes and if only checking for Ubuntu releases, this will cause a regression.

Copy link
Contributor Author

@pupapaik pupapaik Jul 25, 2016

Choose a reason for hiding this comment

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

You are right. It should not use use_carrier for Ubuntu equal or greater than 16.04, because this option is there unknown. I am not sure how is it with Debian releases.

@rallytime rallytime added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jul 19, 2016
bond.update({'use_carrier': '1'})
elif opts['use_carrier'] in _CONFIG_FALSE:
bond.update({'use_carrier': '0'})
if __grains__['os'] == "Ubuntu" and __grains__['osrelease_info'][0] <= 16:
Copy link
Contributor

Choose a reason for hiding this comment

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

@pupapaik Thanks for switching the >= to <= here. That makes more sense.

I am still concerned that the check for Ubuntu in the os grain here will cause problems for Debian releases and I'd like to avoid a regression.

network.managed type bond generate use_carrier
option, which is unknown parameter in ubuntu 16.04

Fixes saltstack#34778
@pupapaik
Copy link
Contributor Author

Can you validate it now? It should affect only Ubuntu Xenial or greater.

@rallytime
Copy link
Contributor

@pupapaik Great! Thank you!

@rallytime rallytime merged commit 21fe1a0 into saltstack:develop Jul 26, 2016
@rallytime rallytime removed the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jul 26, 2016
gitebra pushed a commit to gitebra/salt that referenced this pull request Jul 26, 2016
* upstream/develop: (153 commits)
  Increase all run_script timeouts to 30s (saltstack#34956)
  Fix grains integration tests for Windows (saltstack#34941)
  GCE Cloud tests (saltstack#34871)
  Create regdata before try/except (saltstack#34931)
  Add prune option to reg.list (saltstack#34934)
  Skip inode test in Windows (saltstack#34938)
  Remove use_carrier bond for Ubuntu Xenial later (saltstack#34779)
  Add jid to salt.function orchestration events
  [bashcompletion] replace ticks with bash subshell
  [bashcompletion] ignore salt-key headers
  Add jid to orchestration returns
  Avoid UnboundLocalError in beacons module
  [bash-completion] hide timeout in salt-output
  Master performance improvement (saltstack#34916)
  Update service_rh provider to exclude XenServer >= 7. (saltstack#34915)
  Lint fixes for saltstack#34923
  pv_present should execute pvcreate with -y if existing filesystem signature detected
  Handle exception when no Slack API key was provided
  zone absent with tests
  added zone_present state check
  ...
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.

2 participants