Skip to content

Agent: Update MCC fallback logic #45

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
Merged
shishirb-MSFT merged 3 commits into from
Jun 30, 2021
Merged

Agent: Update MCC fallback logic #45

shishirb-MSFT merged 3 commits into from
Jun 30, 2021

Conversation

@shishirb-MSFT
Copy link
Collaborator

@shishirb-MSFT shishirb-MSFT commented Jun 26, 2021

No description provided.

@shishirb-MSFT
Initial commit: custom fallback logic
3f7af85 
* Ban on fatal errors only.
* 4xx errors are fatal for MCC also.
@shishirb-MSFT
Fixes to MCC fallback
d1e3622 
* ConfigManager handling -1 fallback.
* 4xx errors considered fatal even if fallback override.
* Fix mcc manager tests not appending json files properly.
@shishirb-MSFT shishirb-MSFT requested a review from a team as a code owner June 26, 2021 00:35
jimson-msft
jimson-msft reviewed Jun 28, 2021
View reviewed changes
client-lite/src/config/mcc_manager.h Outdated

private:
std::string address;
std::chrono::steady_clock::time_point timeOfUnban;
Copy link
Contributor

@jimson-msft jimson-msft Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: private members so _ prefix to variable name #Closed

jimson-msft
jimson-msft reviewed Jun 28, 2021
View reviewed changes
client-lite/src/config/mcc_manager.h Outdated

private:
bool _IsFallbackDue(const std::string& host) const;
struct MccHost
Copy link
Contributor

@jimson-msft jimson-msft Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Generally, I see struct's being used as passive data structures (https://en.wikipedia.org/wiki/Passive_data_structure)

Since this contains features of OOP (non-default constructor, access modifiers, member functions) - would it be better to have this as a class? #Closed

Copy link
Collaborator Author

@shishirb-MSFT shishirb-MSFT Jun 28, 2021

Choose a reason for hiding this comment

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

Yes, will change to class.

jimson-msft
jimson-msft reviewed Jun 28, 2021
View reviewed changes
client-lite/src/config/mcc_manager.cpp Outdated
}

bool MCCManager::NoFallback() const
// Returns true if MCC host is banned, false otherwise
Copy link
Contributor

@jimson-msft jimson-msft Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Comment applies to IsBanned? and not ReportHostError correct? Consider deleting anyways cause the function signature explains it well enough #Closed

Copy link
Collaborator Author

@shishirb-MSFT shishirb-MSFT Jun 28, 2021

Choose a reason for hiding this comment

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

Yes, will delete. The method changed mid way through the changes :-)

jimson-msft
jimson-msft reviewed Jun 28, 2021
View reviewed changes
client-lite/src/download/download.cpp Outdated

_fAllowMcc = !_FallbackFromMccDue();

if (_fAllowMcc && !_mccFallbackDue)
Copy link
Contributor

@jimson-msft jimson-msft Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be rewritten as:

if (!_FallbackfromMccDue())

Since _mccFallbackDue is already checked in _FallbackfromMccDue() on 462? #Closed

Copy link
Collaborator Author

@shishirb-MSFT shishirb-MSFT Jun 30, 2021

Choose a reason for hiding this comment

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

Reworked this a bit.

jimson-msft
jimson-msft reviewed Jun 28, 2021
View reviewed changes
client-lite/src/download/download.cpp Outdated
return urlToUse;
}

bool Download::_ShouldFallbackFromMccNow(bool isFatalError) const
Copy link
Contributor

@jimson-msft jimson-msft Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

_ShouldFallbackFromMccNow

Name is kind of confusing, consider _ShouldStopUsingMCC #Closed

Copy link
Collaborator Author

@shishirb-MSFT shishirb-MSFT Jun 30, 2021

Choose a reason for hiding this comment

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

It's not a permanent switch to CDN so I'll rename to _ShouldPauseMccUsage().

jimson-msft
jimson-msft reviewed Jun 29, 2021
View reviewed changes
client-lite/src/download/download.cpp
return static_cast<UINT>(maxNoProgressIntervals);
}

std::string Download::_UpdateConnectionTypeAndGetUrl(bool retryAfterFailure)
Copy link
Contributor

@jimson-msft jimson-msft Jun 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

_UpdateConnectionTypeAndGetUr

So the logic here is to oscillate between cdn and mcc? The download will never time out? #Closed

Copy link
Collaborator Author

@shishirb-MSFT shishirb-MSFT Jun 30, 2021

Choose a reason for hiding this comment

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

Oscillate, yes. The no progress timeout is still enabled, no changes to _SchedProgressTracking().

jimson-msft
jimson-msft previously approved these changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@jimson-msft jimson-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@jimson-msft jimson-msft dismissed their stale review June 29, 2021 00:04

revoking review

jimson-msft
jimson-msft approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@jimson-msft jimson-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

@shishirb-MSFT shishirb-MSFT merged commit dc0fd3c into main Jun 30, 2021
@shishirb-MSFT shishirb-MSFT deleted the user/shishirb/mcc-fallback branch June 30, 2021 16:25
jimson-msft added a commit that referenced this pull request Aug 25, 2021
@jimson-msft @shishirb-MSFT
SDK: Merge main branch into feature/xplat_sdk (#56)
0cce439 
* Use CredScanSuppressions.json instead of inline comments  (#15)

* Add support for additional build types (#28)

Add support for additional build types

* Fix dockerfile for Deb10 (#29)

* Support Ubuntu 20.04 arm64 builds and fix boost log linker issue (#31)

* Update readme with section detailing install (#33)

* Return specific errors from creating and accessing the destination file path (#35)

* Provide better errors for dest file path. Use POSIX APIs.
* Fix tests to account for this.

* deliveryoptimization_set_iot_connection_string returns specific error codes

* Fix WIL link

* Cleanup download in all cases, avoid overwriting existing files (#36)

* SDK: Ensure download is cleaned up in all cases
* Agent: Avoid overwriting existing destination file
* Agent: Fix file path logging in cancel event

* Check binary size growth in azure pipelines (#38)

* Check runs only for minsizerel flavor.
* Baseline number obtained from local dev build.
* Binary size increase of 5% or more results in build failure. Think carefully before increasing the baseline at any time.

* SDK: Prepare to remove cpprestsdk dependency (#39)

* CPortFinder provides port only, address constructed by CHttpClient.
* Remove unused CHttpClient SendRequest method that supported request body.
* Simplify SendRequest. Use boost ptree.

* SDK: Remove cpprestsdk, use custom code with boost.asio (#40)

* Custom, minimal, http parser implementation, on top of Boost.Asio.
* Fix minsizerel build warning in class download.
* Re-enable some tests.

* Strip unneeded symbols from minsizerel binaries (#41)

* Bootstrap only required boost libs
* Strip unneeded symbols from minsizerel binaries.
* Fix deb10 build. boost::asio::io_service backcompat.

* Update limits for binary size check

* Agent: Move log location to /var/log (#43)

* This removes usage of /var/cache
* Group write permissions needed only for the config path

* Make strip_symbols optional (#44)

* Some build environments do not have objcopy installed (like Yocto Zeus)

* Agent: Update MCC fallback logic (#45)

* Custom fallback logic: oscillate between MCC and original host when possible.
* Ban MCC on fatal errors only.
* 4xx errors are fatal for MCC also.
* ConfigManager handling -1 fallback config.
* Fix mcc manager tests not appending json files properly.

* Agent: Reset retry delay on fallback from MCC

* SDK: Add test to exercise new MCC fallback logic

* Increment versions for release v0.7.0 (#50)

agent, sdk: 0.5.0
plugin-apt: 0.3.0

* Fix github_release.yml (#52)

* echo in Release Information task needs the text wrapped in quotes.
* Use minsizerel for building all components.

* Migrate azdo resources to 1ES managed pool (#51)

* use ninja (#53)

* Agent: Replace boost.log usage with custom logger (#54)

* Agent: Remove boost.log requirement from test (#55)

* SDK: Remove inline spaces in favor of nested namespaces (#57)

Co-authored-by: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com>
Co-authored-by: Shishir Bhat <shishirb@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jimson-msft jimson-msft jimson-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shishirb-MSFT @jimson-msft